Support extra custom query parameters in requests to Prometheus backend #6360
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
akstron
commented
Dec 14, 2024
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. |
yurishkuro
reviewed
Dec 14, 2024
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.
Suggested change
| 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.
Suggested change
| 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>
yurishkuro
reviewed
Dec 15, 2024
|
|
||
| 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.
Suggested change
| 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>
90b055f to
046b1b1
Compare
akstron
commented
Dec 16, 2024
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
yurishkuro
approved these changes
Dec 17, 2024
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Alok Kumar Singh <62210712+akstron@users.noreply.github.com>
yurishkuro
approved these changes
Dec 19, 2024
|
Thanks! |
4 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2025
## Which problem is this PR solving? - All config options except for `extra_query_parameters` are configurable via CLI parameters ## Description of the changes - Support configuring prometheus.extra_query_parameters via CLI Related: #6360 ## How was this change tested? - unit test ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
andreasgerstmayr
added a commit
to os-observability/jaeger
that referenced
this pull request
Mar 25, 2025
…tracing#6931) ## Which problem is this PR solving? - All config options except for `extra_query_parameters` are configurable via CLI parameters ## Description of the changes - Support configuring prometheus.extra_query_parameters via CLI Related: jaegertracing#6360 ## How was this change tested? - unit test ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
amilbcahat
pushed a commit
to amilbcahat/jaeger
that referenced
this pull request
May 4, 2025
…tracing#6931) ## Which problem is this PR solving? - All config options except for `extra_query_parameters` are configurable via CLI parameters ## Description of the changes - Support configuring prometheus.extra_query_parameters via CLI Related: jaegertracing#6360 ## How was this change tested? - unit test ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
amilbcahat
pushed a commit
to amilbcahat/jaeger
that referenced
this pull request
May 4, 2025
…tracing#6931) ## Which problem is this PR solving? - All config options except for `extra_query_parameters` are configurable via CLI parameters ## Description of the changes - Support configuring prometheus.extra_query_parameters via CLI Related: jaegertracing#6360 ## How was this change tested? - unit test ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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