From 92917a49451e28de95935577d22a67728d097681 Mon Sep 17 00:00:00 2001 From: Matt Mix Date: Thu, 31 Jul 2025 16:49:25 -0400 Subject: [PATCH] chore(metrics): Updating so that request duration uses the common metrics registry. --- docs/monitoring/metrics.md | 2 +- internal/gen/docs/metrics/main_test.go | 2 +- pkg/http/http.go | 21 +++------- pkg/http/http_test.go | 22 ----------- pkg/metrics/metrics.go | 4 +- pkg/metrics/metrics_test.go | 3 +- pkg/metrics/models.go | 39 +++++++++++++++++++ pkg/metrics/models_test.go | 53 ++++++++++++++++++++++++++ 8 files changed, 104 insertions(+), 42 deletions(-) diff --git a/docs/monitoring/metrics.md b/docs/monitoring/metrics.md index 3da6d52af..2d317ea6b 100644 --- a/docs/monitoring/metrics.md +++ b/docs/monitoring/metrics.md @@ -26,6 +26,7 @@ curl https://localhost:7979/metrics | last_sync_timestamp_seconds | Gauge | controller | Timestamp of last successful sync with the DNS provider | | no_op_runs_total | Counter | controller | Number of reconcile loops ending up with no changes on the DNS provider side. | | verified_records | Gauge | controller | Number of DNS records that exists both in source and registry (vector). | +| request_duration_seconds | Summaryvec | http | The HTTP request latencies in seconds. | | cache_apply_changes_calls | Counter | provider | Number of calls to the provider cache ApplyChanges. | | cache_records_calls | Counter | provider | Number of calls to the provider cache Records list. | | endpoints_total | Gauge | registry | Number of Endpoints in the registry | @@ -76,7 +77,6 @@ curl https://localhost:7979/metrics | go_memstats_sys_bytes | | go_sched_gomaxprocs_threads | | go_threads | -| http_request_duration_seconds | | process_cpu_seconds_total | | process_max_fds | | process_network_receive_bytes_total | diff --git a/internal/gen/docs/metrics/main_test.go b/internal/gen/docs/metrics/main_test.go index bea1d75e8..dd9884e95 100644 --- a/internal/gen/docs/metrics/main_test.go +++ b/internal/gen/docs/metrics/main_test.go @@ -37,7 +37,7 @@ func TestComputeMetrics(t *testing.T) { t.Errorf("Expected not empty metrics registry, got %d", len(reg.Metrics)) } - assert.Len(t, reg.Metrics, 20) + assert.Len(t, reg.Metrics, 21) } func TestGenerateMarkdownTableRenderer(t *testing.T) { diff --git a/pkg/http/http.go b/pkg/http/http.go index b6069f1a5..b78a763a1 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -21,14 +21,15 @@ package http import ( "fmt" "net/http" - "strings" "time" "github.com/prometheus/client_golang/prometheus" + + "sigs.k8s.io/external-dns/pkg/metrics" ) var ( - requestDuration = prometheus.NewSummaryVec( + RequestDurationMetric = metrics.NewSummaryVecWithOpts( prometheus.SummaryOpts{ Name: "request_duration_seconds", Help: "The HTTP request latencies in seconds.", @@ -41,7 +42,7 @@ var ( ) func init() { - prometheus.MustRegister(requestDuration) + metrics.RegisterMetric.MustRegister(RequestDurationMetric) } type CustomRoundTripper struct { @@ -61,15 +62,8 @@ func (r *CustomRoundTripper) RoundTrip(req *http.Request) (*http.Response, error if resp != nil { status = fmt.Sprintf("%d", resp.StatusCode) } + RequestDurationMetric.SetWithLabels(time.Since(start).Seconds(), req.URL.Scheme, req.URL.Host, metrics.PathProcessor(req.URL.Path), req.Method, status) - labels := prometheus.Labels{ - "scheme": req.URL.Scheme, - "host": req.URL.Host, - "path": pathProcessor(req.URL.Path), - "method": req.Method, - "status": status, - } - requestDuration.With(labels).Observe(time.Since(start).Seconds()) return resp, err } @@ -90,8 +84,3 @@ func NewInstrumentedTransport(next http.RoundTripper) http.RoundTripper { return &CustomRoundTripper{next: next} } - -func pathProcessor(path string) string { - parts := strings.Split(path, "/") - return parts[len(parts)-1] -} diff --git a/pkg/http/http_test.go b/pkg/http/http_test.go index cd17f3873..1e46e2292 100644 --- a/pkg/http/http_test.go +++ b/pkg/http/http_test.go @@ -57,25 +57,3 @@ func TestNewInstrumentedClient(t *testing.T) { _, ok = result2.Transport.(*CustomRoundTripper) require.True(t, ok) } - -func TestPathProcessor(t *testing.T) { - tests := []struct { - input string - expected string - }{ - {"/foo/bar", "bar"}, - {"/foo/", ""}, - {"/", ""}, - {"", ""}, - {"/foo/bar/baz", "baz"}, - {"foo/bar", "bar"}, - {"foo", "foo"}, - {"foo/", ""}, - } - - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - require.Equal(t, tt.expected, pathProcessor(tt.input)) - }) - } -} diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 73e6ea5d1..93bf4acec 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -73,7 +73,7 @@ func NewMetricsRegister() *MetricRegistry { // } func (m *MetricRegistry) MustRegister(cs IMetric) { switch v := cs.(type) { - case CounterMetric, GaugeMetric, CounterVecMetric, GaugeVecMetric, GaugeFuncMetric: + case CounterMetric, GaugeMetric, SummaryVecMetric, CounterVecMetric, GaugeVecMetric, GaugeFuncMetric: if _, exists := m.mName[cs.Get().FQDN]; exists { return } else { @@ -85,6 +85,8 @@ func (m *MetricRegistry) MustRegister(cs IMetric) { m.Registerer.MustRegister(metric.Counter) case GaugeMetric: m.Registerer.MustRegister(metric.Gauge) + case SummaryVecMetric: + m.Registerer.MustRegister(metric.SummaryVec) case GaugeVecMetric: m.Registerer.MustRegister(metric.Gauge) case CounterVecMetric: diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index 16cf52f57..b7a0aca49 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -61,8 +61,9 @@ func TestMustRegister(t *testing.T) { NewCounterWithOpts(prometheus.CounterOpts{Name: "test_counter_3"}), NewCounterVecWithOpts(prometheus.CounterOpts{Name: "test_counter_vec_3"}, []string{"label"}), NewGaugedVectorOpts(prometheus.GaugeOpts{Name: "test_gauge_v_3"}, []string{"label"}), + NewSummaryVecWithOpts(prometheus.SummaryOpts{Name: "test_summary_v_3"}, []string{"label"}), }, - expected: 4, + expected: 5, }, { name: "unsupported metric", diff --git a/pkg/metrics/models.go b/pkg/metrics/models.go index 8c8fb4d4f..11fd47419 100644 --- a/pkg/metrics/models.go +++ b/pkg/metrics/models.go @@ -176,3 +176,42 @@ func NewGaugeFuncMetric(opts prometheus.GaugeOpts) GaugeFuncMetric { GaugeFunc: prometheus.NewGaugeFunc(opts, func() float64 { return 1 }), } } + +type SummaryVecMetric struct { + Metric + SummaryVec prometheus.SummaryVec +} + +func (s SummaryVecMetric) Get() *Metric { + return &s.Metric +} + +// SetWithLabels sets the value of the SummaryVec metric for the specified label values. +// All label values are converted to lowercase before being applied. +func (s SummaryVecMetric) SetWithLabels(value float64, lvs ...string) { + for i, v := range lvs { + lvs[i] = strings.ToLower(v) + } + + s.SummaryVec.WithLabelValues(lvs...).Observe(value) +} + +func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labelNames []string) SummaryVecMetric { + opts.Namespace = Namespace + return SummaryVecMetric{ + Metric: Metric{ + Type: "summaryVec", + Name: opts.Name, + FQDN: fmt.Sprintf("%s_%s", opts.Subsystem, opts.Name), + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Help: opts.Help, + }, + SummaryVec: *prometheus.NewSummaryVec(opts, labelNames), + } +} + +func PathProcessor(path string) string { + parts := strings.Split(path, "/") + return parts[len(parts)-1] +} diff --git a/pkg/metrics/models_test.go b/pkg/metrics/models_test.go index 738f9f6ed..dc9c7a7db 100644 --- a/pkg/metrics/models_test.go +++ b/pkg/metrics/models_test.go @@ -23,6 +23,7 @@ import ( "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewGaugeWithOpts(t *testing.T) { @@ -128,3 +129,55 @@ func TestNewBuildInfoCollector(t *testing.T) { assert.Equal(t, "external_dns_build_info", reflect.ValueOf(desc).Elem().FieldByName("fqName").String()) assert.Contains(t, desc.String(), "version=\"0.0.1\"") } + +func TestSummaryV_SetWithLabels(t *testing.T) { + opts := prometheus.SummaryOpts{ + Name: "test_summaryVec", + Namespace: "test_ns", + Subsystem: "test_sub", + Help: "help text", + } + sv := NewSummaryVecWithOpts(opts, []string{"label1", "label2"}) + + sv.SetWithLabels(5.01, "Alpha", "BETA") + + reg := prometheus.NewRegistry() + reg.MustRegister(sv.SummaryVec) + + metricsFamilies, err := reg.Gather() + assert.NoError(t, err) + assert.Len(t, metricsFamilies, 1) + + s, err := sv.SummaryVec.GetMetricWithLabelValues("alpha", "beta") + assert.NoError(t, err) + metricsFamilies, err = reg.Gather() + + s.Observe(5.21) + metricsFamilies, err = reg.Gather() + assert.NoError(t, err) + + assert.InDelta(t, 10.22, *metricsFamilies[0].Metric[0].Summary.SampleSum, 0.01) + assert.Len(t, metricsFamilies[0].Metric[0].Label, 2) +} + +func TestPathProcessor(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"/foo/bar", "bar"}, + {"/foo/", ""}, + {"/", ""}, + {"", ""}, + {"/foo/bar/baz", "baz"}, + {"foo/bar", "bar"}, + {"foo", "foo"}, + {"foo/", ""}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + require.Equal(t, tt.expected, PathProcessor(tt.input)) + }) + } +}