Support extra custom query parameters in requests to Prometheus backend #6360
Conversation
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6360 +/- ##
==========================================
+ Coverage 96.14% 96.22% +0.08%
==========================================
Files 357 361 +4
Lines 20547 20623 +76
==========================================
+ Hits 19755 19845 +90
+ Misses 605 595 -10
+ Partials 187 183 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/prometheus/config/config.go
Outdated
| LatencyUnit string `mapstructure:"latency_unit"` | ||
| NormalizeCalls bool `mapstructure:"normalize_calls"` | ||
| NormalizeDuration bool `mapstructure:"normalize_duration"` | ||
| AdditionalParameters map[string]string `mapstructure:"additional_parameters"` |
There was a problem hiding this comment.
| AdditionalParameters map[string]string `mapstructure:"additional_parameters"` | |
| ExtraQueryParams map[string]string `mapstructure:"extra_query_parameters"` |
There was a problem hiding this comment.
please add a comment describing the purpose
|
|
||
| promClient struct { | ||
| api.Client | ||
| params map[string]string |
There was a problem hiding this comment.
| params map[string]string | |
| extraParams map[string]string |
|
|
||
| // NewMetricsReader returns a new MetricsReader. | ||
| func NewMetricsReader(cfg config.Configuration, logger *zap.Logger, tracer trace.TracerProvider) (*MetricsReader, error) { | ||
| logger.Info("Creating metrics reader", zap.Any("configuration", cfg)) |
There was a problem hiding this comment.
not a good idea to log configuration as it may contain credentials
There was a problem hiding this comment.
This code was already present before I refactored it. Removed it as suggested.
| LatencyUnit: defaultLatencyUnit, | ||
| NormalizeCalls: defaultNormalizeCalls, | ||
| NormalizeDuration: defaultNormalizeCalls, | ||
| AdditionalParameters: nil, |
There was a problem hiding this comment.
nil is the default value, no effect
There was a problem hiding this comment.
I see. Thanks. Removed it, as nil should be the default value.
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
akstron
changed the title
|
|
||
| query := u.Query() | ||
| for k, v := range p.extraParams { | ||
| query.Set(k, v) |
There was a problem hiding this comment.
why Set and not Add? We are not asked to replace params, only append them.
There was a problem hiding this comment.
Makes sense. Changed it to Add and updated test accordingly.
|
|
||
| func createPromClient(logger *zap.Logger, cfg config.Configuration) (api.Client, error) { |
There was a problem hiding this comment.
you don't need logger here, and neither does getHTTPRoundTripper
There was a problem hiding this comment.
refactored to remove logger from both. Thanks
|
|
||
| logger := zap.NewNop() | ||
| cfg := config.Configuration{ | ||
| ServerURL: "http://localhost:1234", |
There was a problem hiding this comment.
| ServerURL: "http://localhost:1234", | |
| ServerURL: "http://localhost:1234?param1=value0", |
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Alok Kumar Singh <62210712+akstron@users.noreply.github.com>
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
akstron
force-pushed
the
prometheus-additional-query-parameter
branch
from
90b055f to
046b1b1
Compare
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Alok Kumar Singh <62210712+akstron@users.noreply.github.com>
yurishkuro
changed the title
|
Thanks! |
Which problem is this PR solving?
Description of the changes
promClientto decorate url with additional parametersHow was this change tested?
TestCreatePromClientWithAdditionalParametersChecklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test