-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support extra custom query parameters in requests to Prometheus backend #6360
Changes from 2 commits
144ba92
f7daaea
5023abd
d4fb513
046b1b1
4f51692
be61244
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |||||
"fmt" | ||||||
"net" | ||||||
"net/http" | ||||||
"net/url" | ||||||
"os" | ||||||
"path/filepath" | ||||||
"strings" | ||||||
|
@@ -60,16 +61,32 @@ type ( | |||||
metricDesc string | ||||||
buildPromQuery func(p promQueryParams) string | ||||||
} | ||||||
|
||||||
promClient struct { | ||||||
api.Client | ||||||
params map[string]string | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||
} | ||||||
) | ||||||
|
||||||
// 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)) | ||||||
// URL decorator enables adding additional query parameters to the request sent to prometheus backend | ||||||
func (p promClient) URL(ep string, args map[string]string) *url.URL { | ||||||
u := p.Client.URL(ep, args) | ||||||
|
||||||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
query := u.Query() | ||||||
for k, v := range p.params { | ||||||
query.Set(k, v) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why Set and not Add? We are not asked to replace params, only append them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Changed it to Add and updated test accordingly. |
||||||
} | ||||||
u.RawQuery = query.Encode() | ||||||
|
||||||
return u | ||||||
} | ||||||
|
||||||
func createPromClient(logger *zap.Logger, cfg config.Configuration) (api.Client, error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't need logger here, and neither does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. refactored to remove logger from both. Thanks |
||||||
roundTripper, err := getHTTPRoundTripper(&cfg, logger) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
|
||||||
client, err := api.NewClient(api.Config{ | ||||||
Address: cfg.ServerURL, | ||||||
RoundTripper: roundTripper, | ||||||
|
@@ -78,10 +95,27 @@ func NewMetricsReader(cfg config.Configuration, logger *zap.Logger, tracer trace | |||||
return nil, fmt.Errorf("failed to initialize prometheus client: %w", err) | ||||||
} | ||||||
|
||||||
customClient := promClient{ | ||||||
Client: client, | ||||||
params: cfg.AdditionalParameters, | ||||||
} | ||||||
|
||||||
return customClient, nil | ||||||
} | ||||||
|
||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a good idea to log configuration as it may contain credentials There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was already present before I refactored it. Removed it as suggested. |
||||||
|
||||||
const operationLabel = "span_name" | ||||||
|
||||||
promClient, err := createPromClient(logger, cfg) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
|
||||||
mr := &MetricsReader{ | ||||||
client: promapi.NewAPI(client), | ||||||
client: promapi.NewAPI(promClient), | ||||||
logger: logger, | ||||||
tracer: tracer.Tracer("prom-metrics-reader"), | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,10 +51,11 @@ func DefaultConfig() config.Configuration { | |
ServerURL: defaultServerURL, | ||
ConnectTimeout: defaultConnectTimeout, | ||
|
||
MetricNamespace: defaultMetricNamespace, | ||
LatencyUnit: defaultLatencyUnit, | ||
NormalizeCalls: defaultNormalizeCalls, | ||
NormalizeDuration: defaultNormalizeCalls, | ||
MetricNamespace: defaultMetricNamespace, | ||
LatencyUnit: defaultLatencyUnit, | ||
NormalizeCalls: defaultNormalizeCalls, | ||
NormalizeDuration: defaultNormalizeCalls, | ||
AdditionalParameters: nil, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nil is the default value, no effect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Thanks. Removed it, as nil should be the default value. |
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment describing the purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.