From 295293c53c68df4dbc896082a00a5bc3d532cfc9 Mon Sep 17 00:00:00 2001
From: Raghuram Kannan <78465537+FlamingSaint@users.noreply.github.com>
Date: Mon, 8 Jul 2024 23:30:50 +0530
Subject: [PATCH] Cleanup the Prometheus config (#5720)
## Which problem is this PR solving?
- Part of #5632
## Description of the changes
- Cleaned up the Prometheus config
- Removed namespaceConfig as its not needed for a metrics storage
## How was this change tested?
- `make test`
## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
- for `jaeger`: `make lint test`
- for `jaeger-ui`: `yarn lint` and `yarn test`
---------
Signed-off-by: FlamingSaint
---
pkg/version/build_test.go | 2 +-
plugin/metrics/prometheus/factory.go | 4 +-
plugin/metrics/prometheus/factory_test.go | 27 +++++-----
plugin/metrics/prometheus/options.go | 62 ++++++++++-------------
4 files changed, 43 insertions(+), 52 deletions(-)
diff --git a/pkg/version/build_test.go b/pkg/version/build_test.go
index 94b9360f50c..d08224e188f 100644
--- a/pkg/version/build_test.go
+++ b/pkg/version/build_test.go
@@ -43,4 +43,4 @@ func TestString(t *testing.T) {
}
expectedOutput := "git-commit=foobar, git-version=v1.2.3, build-date=2024-01-04"
assert.Equal(t, expectedOutput, test.String())
-}
\ No newline at end of file
+}
diff --git a/plugin/metrics/prometheus/factory.go b/plugin/metrics/prometheus/factory.go
index 60452061d67..a806942eb46 100644
--- a/plugin/metrics/prometheus/factory.go
+++ b/plugin/metrics/prometheus/factory.go
@@ -40,7 +40,7 @@ type Factory struct {
func NewFactory() *Factory {
return &Factory{
tracer: otel.GetTracerProvider(),
- options: NewOptions("prometheus"),
+ options: NewOptions(),
}
}
@@ -64,5 +64,5 @@ func (f *Factory) Initialize(logger *zap.Logger) error {
// CreateMetricsReader implements storage.MetricsFactory.
func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) {
- return prometheusstore.NewMetricsReader(f.options.Primary.Configuration, f.logger, f.tracer)
+ return prometheusstore.NewMetricsReader(f.options.Configuration, f.logger, f.tracer)
}
diff --git a/plugin/metrics/prometheus/factory_test.go b/plugin/metrics/prometheus/factory_test.go
index dc03edc2304..bd49eca32d5 100644
--- a/plugin/metrics/prometheus/factory_test.go
+++ b/plugin/metrics/prometheus/factory_test.go
@@ -34,14 +34,13 @@ func TestPrometheusFactory(t *testing.T) {
f := NewFactory()
require.NoError(t, f.Initialize(zap.NewNop()))
assert.NotNil(t, f.logger)
- assert.Equal(t, "prometheus", f.options.Primary.namespace)
listener, err := net.Listen("tcp", "localhost:")
require.NoError(t, err)
assert.NotNil(t, listener)
defer listener.Close()
- f.options.Primary.ServerURL = "http://" + listener.Addr().String()
+ f.options.ServerURL = "http://" + listener.Addr().String()
reader, err := f.CreateMetricsReader()
require.NoError(t, err)
@@ -50,11 +49,11 @@ func TestPrometheusFactory(t *testing.T) {
func TestWithDefaultConfiguration(t *testing.T) {
f := NewFactory()
- assert.Equal(t, "http://localhost:9090", f.options.Primary.ServerURL)
- assert.Equal(t, 30*time.Second, f.options.Primary.ConnectTimeout)
+ assert.Equal(t, "http://localhost:9090", f.options.ServerURL)
+ assert.Equal(t, 30*time.Second, f.options.ConnectTimeout)
- assert.Empty(t, f.options.Primary.MetricNamespace)
- assert.Equal(t, "ms", f.options.Primary.LatencyUnit)
+ assert.Empty(t, f.options.MetricNamespace)
+ assert.Equal(t, "ms", f.options.LatencyUnit)
}
func TestWithConfiguration(t *testing.T) {
@@ -69,10 +68,10 @@ func TestWithConfiguration(t *testing.T) {
})
require.NoError(t, err)
f.InitFromViper(v, zap.NewNop())
- assert.Equal(t, "http://localhost:1234", f.options.Primary.ServerURL)
- assert.Equal(t, 5*time.Second, f.options.Primary.ConnectTimeout)
- assert.Equal(t, "test/test_file.txt", f.options.Primary.TokenFilePath)
- assert.False(t, f.options.Primary.TokenOverrideFromContext)
+ assert.Equal(t, "http://localhost:1234", f.options.ServerURL)
+ assert.Equal(t, 5*time.Second, f.options.ConnectTimeout)
+ assert.Equal(t, "test/test_file.txt", f.options.TokenFilePath)
+ assert.False(t, f.options.TokenOverrideFromContext)
})
t.Run("with space in token file path", func(t *testing.T) {
f := NewFactory()
@@ -82,7 +81,7 @@ func TestWithConfiguration(t *testing.T) {
})
require.NoError(t, err)
f.InitFromViper(v, zap.NewNop())
- assert.Equal(t, "test/ test file.txt", f.options.Primary.TokenFilePath)
+ assert.Equal(t, "test/ test file.txt", f.options.TokenFilePath)
})
t.Run("with custom configuration of prometheus.query", func(t *testing.T) {
f := NewFactory()
@@ -93,8 +92,8 @@ func TestWithConfiguration(t *testing.T) {
})
require.NoError(t, err)
f.InitFromViper(v, zap.NewNop())
- assert.Equal(t, "mynamespace", f.options.Primary.MetricNamespace)
- assert.Equal(t, "ms", f.options.Primary.LatencyUnit)
+ assert.Equal(t, "mynamespace", f.options.MetricNamespace)
+ assert.Equal(t, "ms", f.options.LatencyUnit)
})
t.Run("with invalid prometheus.query.duration-unit", func(t *testing.T) {
defer func() {
@@ -110,7 +109,7 @@ func TestWithConfiguration(t *testing.T) {
})
require.NoError(t, err)
f.InitFromViper(v, zap.NewNop())
- require.Empty(t, f.options.Primary.LatencyUnit)
+ require.Empty(t, f.options.LatencyUnit)
})
}
diff --git a/plugin/metrics/prometheus/options.go b/plugin/metrics/prometheus/options.go
index a9f8f98956b..80cf09e3070 100644
--- a/plugin/metrics/prometheus/options.go
+++ b/plugin/metrics/prometheus/options.go
@@ -27,6 +27,8 @@ import (
)
const (
+ prefix = "prometheus"
+
suffixServerURL = ".server-url"
suffixConnectTimeout = ".connect-timeout"
suffixTokenFilePath = ".token-file"
@@ -48,18 +50,13 @@ const (
defaultNormalizeDuration = false
)
-type namespaceConfig struct {
- config.Configuration `mapstructure:",squash"`
- namespace string
-}
-
// Options stores the configuration entries for this storage.
type Options struct {
- Primary namespaceConfig `mapstructure:",squash"`
+ config.Configuration `mapstructure:",squash"`
}
// NewOptions creates a new Options struct.
-func NewOptions(primaryNamespace string) *Options {
+func NewOptions() *Options {
defaultConfig := config.Configuration{
ServerURL: defaultServerURL,
ConnectTimeout: defaultConnectTimeout,
@@ -71,75 +68,70 @@ func NewOptions(primaryNamespace string) *Options {
}
return &Options{
- Primary: namespaceConfig{
- Configuration: defaultConfig,
- namespace: primaryNamespace,
- },
+ Configuration: defaultConfig,
}
}
// AddFlags from this storage to the CLI.
func (opt *Options) AddFlags(flagSet *flag.FlagSet) {
- nsConfig := &opt.Primary
- flagSet.String(nsConfig.namespace+suffixServerURL, defaultServerURL,
+ flagSet.String(prefix+suffixServerURL, defaultServerURL,
"The Prometheus server's URL, must include the protocol scheme e.g. http://localhost:9090")
- flagSet.Duration(nsConfig.namespace+suffixConnectTimeout, defaultConnectTimeout,
+ flagSet.Duration(prefix+suffixConnectTimeout, defaultConnectTimeout,
"The period to wait for a connection to Prometheus when executing queries.")
- flagSet.String(nsConfig.namespace+suffixTokenFilePath, defaultTokenFilePath,
+ flagSet.String(prefix+suffixTokenFilePath, defaultTokenFilePath,
"The path to a file containing the bearer token which will be included when executing queries against the Prometheus API.")
- flagSet.Bool(nsConfig.namespace+suffixOverrideFromContext, true,
+ flagSet.Bool(prefix+suffixOverrideFromContext, true,
"Whether the bearer token should be overridden from context (incoming request)")
- flagSet.String(nsConfig.namespace+suffixMetricNamespace, defaultMetricNamespace,
+ flagSet.String(prefix+suffixMetricNamespace, defaultMetricNamespace,
`The metric namespace that is prefixed to the metric name. A '.' separator will be added between `+
`the namespace and the metric name.`)
- flagSet.String(nsConfig.namespace+suffixLatencyUnit, defaultLatencyUnit,
+ flagSet.String(prefix+suffixLatencyUnit, defaultLatencyUnit,
`The units used for the "latency" histogram. It can be either "ms" or "s" and should be consistent with the `+
`histogram unit value set in the spanmetrics connector (see: `+
`https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/connector/spanmetricsconnector#configurations). `+
`This also helps jaeger-query determine the metric name when querying for "latency" metrics.`)
- flagSet.Bool(nsConfig.namespace+suffixNormalizeCalls, defaultNormalizeCalls,
+ flagSet.Bool(prefix+suffixNormalizeCalls, defaultNormalizeCalls,
`Whether to normalize the "calls" metric name according to `+
`https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheus/README.md. `+
`For example: `+
`"calls" (not normalized) -> "calls_total" (normalized), `)
- flagSet.Bool(nsConfig.namespace+suffixNormalizeDuration, defaultNormalizeDuration,
+ flagSet.Bool(prefix+suffixNormalizeDuration, defaultNormalizeDuration,
`Whether to normalize the "duration" metric name according to `+
`https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheus/README.md. `+
`For example: `+
`"duration_bucket" (not normalized) -> "duration_milliseconds_bucket (normalized)"`)
- nsConfig.getTLSFlagsConfig().AddFlags(flagSet)
+ opt.getTLSFlagsConfig().AddFlags(flagSet)
}
// InitFromViper initializes the options struct with values from Viper.
func (opt *Options) InitFromViper(v *viper.Viper) error {
- cfg := &opt.Primary
- cfg.ServerURL = stripWhiteSpace(v.GetString(cfg.namespace + suffixServerURL))
- cfg.ConnectTimeout = v.GetDuration(cfg.namespace + suffixConnectTimeout)
- cfg.TokenFilePath = v.GetString(cfg.namespace + suffixTokenFilePath)
+ opt.ServerURL = stripWhiteSpace(v.GetString(prefix + suffixServerURL))
+ opt.ConnectTimeout = v.GetDuration(prefix + suffixConnectTimeout)
+ opt.TokenFilePath = v.GetString(prefix + suffixTokenFilePath)
- cfg.MetricNamespace = v.GetString(cfg.namespace + suffixMetricNamespace)
- cfg.LatencyUnit = v.GetString(cfg.namespace + suffixLatencyUnit)
- cfg.NormalizeCalls = v.GetBool(cfg.namespace + suffixNormalizeCalls)
- cfg.NormalizeDuration = v.GetBool(cfg.namespace + suffixNormalizeDuration)
- cfg.TokenOverrideFromContext = v.GetBool(cfg.namespace + suffixOverrideFromContext)
+ opt.MetricNamespace = v.GetString(prefix + suffixMetricNamespace)
+ opt.LatencyUnit = v.GetString(prefix + suffixLatencyUnit)
+ opt.NormalizeCalls = v.GetBool(prefix + suffixNormalizeCalls)
+ opt.NormalizeDuration = v.GetBool(prefix + suffixNormalizeDuration)
+ opt.TokenOverrideFromContext = v.GetBool(prefix + suffixOverrideFromContext)
isValidUnit := map[string]bool{"ms": true, "s": true}
- if _, ok := isValidUnit[cfg.LatencyUnit]; !ok {
- return fmt.Errorf(`duration-unit must be one of "ms" or "s", not %q`, cfg.LatencyUnit)
+ if _, ok := isValidUnit[opt.LatencyUnit]; !ok {
+ return fmt.Errorf(`duration-unit must be one of "ms" or "s", not %q`, opt.LatencyUnit)
}
var err error
- cfg.TLS, err = cfg.getTLSFlagsConfig().InitFromViper(v)
+ opt.TLS, err = opt.getTLSFlagsConfig().InitFromViper(v)
if err != nil {
return fmt.Errorf("failed to process Prometheus TLS options: %w", err)
}
return nil
}
-func (config *namespaceConfig) getTLSFlagsConfig() tlscfg.ClientFlagsConfig {
+func (*Options) getTLSFlagsConfig() tlscfg.ClientFlagsConfig {
return tlscfg.ClientFlagsConfig{
- Prefix: config.namespace,
+ Prefix: prefix,
}
}