Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions pkg/prometheus/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ type Configuration struct {
TokenFilePath string `mapstructure:"token_file_path"`
TokenOverrideFromContext bool `mapstructure:"token_override_from_context"`

MetricNamespace string `mapstructure:"metric_namespace"`
LatencyUnit string `mapstructure:"latency_unit"`
NormalizeCalls bool `mapstructure:"normalize_calls"`
NormalizeDuration bool `mapstructure:"normalize_duration"`
MetricNamespace string `mapstructure:"metric_namespace"`
LatencyUnit string `mapstructure:"latency_unit"`
NormalizeCalls bool `mapstructure:"normalize_calls"`
NormalizeDuration bool `mapstructure:"normalize_duration"`
AdditionalParameters map[string]string `mapstructure:"additional_parameters"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AdditionalParameters map[string]string `mapstructure:"additional_parameters"`
ExtraQueryParams map[string]string `mapstructure:"extra_query_parameters"`

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

func (c *Configuration) Validate() error {
Expand Down
42 changes: 38 additions & 4 deletions plugin/metricstore/prometheus/metricstore/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"net"
"net/http"
"net/url"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -60,16 +61,32 @@ type (
metricDesc string
buildPromQuery func(p promQueryParams) string
}

promClient struct {
api.Client
params map[string]string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
params map[string]string
extraParams map[string]string

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need logger here, and neither does getHTTPRoundTripper

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a good idea to log configuration as it may contain credentials

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"),

Expand Down
25 changes: 25 additions & 0 deletions plugin/metricstore/prometheus/metricstore/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,31 @@ func TestInvalidCertFile(t *testing.T) {
assert.Nil(t, reader)
}

func TestCreatePromClientWithAdditionalParameters(t *testing.T) {
expParams := map[string]string{
"param1": "value1",
"param2": "value2",
}

logger := zap.NewNop()
cfg := config.Configuration{
ServerURL: "http://localhost:1234",
AdditionalParameters: expParams,
}

customClient, err := createPromClient(logger, cfg)
require.NoError(t, err)

u := customClient.URL("", nil)

q := u.Query()

for k, v := range expParams {
recV := q.Get(k)
require.Equal(t, v, recV)
}
}

func startMockPrometheusServer(t *testing.T, wantPromQlQuery string, wantWarnings []string) *httptest.Server {
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if len(wantWarnings) > 0 {
Expand Down
9 changes: 5 additions & 4 deletions plugin/metricstore/prometheus/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil is the default value, no effect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks. Removed it, as nil should be the default value.

}
}

Expand Down
Loading