diff --git a/pkg/http/http.go b/pkg/http/http.go index 78cc05380b..c52d30a8ab 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -29,7 +29,6 @@ import ( ) var ( - RequestDurationLabels = metrics.NewLabels([]string{"scheme", "host", "path", "method", "status"}) RequestDurationMetric = metrics.NewSummaryVecWithOpts( prometheus.SummaryOpts{ Name: "request_duration_seconds", @@ -38,7 +37,7 @@ var ( ConstLabels: prometheus.Labels{"handler": "instrumented_http"}, Objectives: map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001}, }, - *RequestDurationLabels, + []string{metrics.LabelScheme, metrics.LabelHost, metrics.LabelPath, metrics.LabelMethod, metrics.LabelStatus}, ) ) @@ -64,15 +63,13 @@ func (r *CustomRoundTripper) RoundTrip(req *http.Request) (*http.Response, error status = fmt.Sprintf("%d", resp.StatusCode) } - RequestDurationLabels.WithOptions( - metrics.WithLabel("scheme", req.URL.Scheme), - metrics.WithLabel("host", req.URL.Host), - metrics.WithLabel("path", metrics.PathProcessor(req.URL.Path)), - metrics.WithLabel("method", req.Method), - metrics.WithLabel("status", status), - ) - - RequestDurationMetric.SetWithLabels(time.Since(start).Seconds(), RequestDurationLabels) + RequestDurationMetric.SetWithLabels(time.Since(start).Seconds(), metrics.Labels{ + metrics.LabelScheme: req.URL.Scheme, + metrics.LabelHost: req.URL.Host, + metrics.LabelPath: metrics.PathProcessor(req.URL.Path), + metrics.LabelMethod: req.Method, + metrics.LabelStatus: status, + }) return resp, err } diff --git a/pkg/http/http_benchmark_test.go b/pkg/http/http_benchmark_test.go new file mode 100644 index 0000000000..efdf4e216e --- /dev/null +++ b/pkg/http/http_benchmark_test.go @@ -0,0 +1,97 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package http + +import ( + "bytes" + "io" + "net/http" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type roundTripFunc func(req *http.Request) *http.Response + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req), nil +} + +// newTestClient returns *http.client with Transport replaced to avoid making real calls +func newTestClient(fn roundTripFunc) *http.Client { + return &http.Client{ + Transport: NewInstrumentedTransport(fn), + } +} + +type apiUnderTest struct { + client *http.Client + baseURL string +} + +func (api *apiUnderTest) doStuff() ([]byte, error) { + resp, err := api.client.Get(api.baseURL + "/some/path") + if err != nil { + return nil, err + } + defer resp.Body.Close() + return io.ReadAll(resp.Body) +} + +func BenchmarkRoundTripper(b *testing.B) { + client := newTestClient(func(req *http.Request) *http.Response { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(`OK`)), + Header: make(http.Header), + } + }) + + for b.Loop() { + api := apiUnderTest{client, "http://example.com"} + body, err := api.doStuff() + require.NoError(b, err) + assert.Equal(b, []byte("OK"), body) + } +} + +func TestRoundTripper_Concurrent(t *testing.T) { + client := newTestClient(func(req *http.Request) *http.Response { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(`OK`)), + Header: make(http.Header), + } + }) + api := &apiUnderTest{client: client, baseURL: "http://example.com"} + + const numGoroutines = 100 + var wg sync.WaitGroup + wg.Add(numGoroutines) + + for i := 0; i < numGoroutines; i++ { + go func() { + defer wg.Done() + body, err := api.doStuff() + assert.NoError(t, err) + assert.Equal(t, []byte("OK"), body) + }() + } + wg.Wait() +} diff --git a/pkg/metrics/labels.go b/pkg/metrics/labels.go index d6381ed8ea..bf16550f6a 100644 --- a/pkg/metrics/labels.go +++ b/pkg/metrics/labels.go @@ -17,62 +17,15 @@ limitations under the License. package metrics import ( - "sort" - "strings" - - "github.com/sirupsen/logrus" + "github.com/prometheus/client_golang/prometheus" ) -type Labels struct { - values map[string]string -} - -func (labels *Labels) GetKeysInOrder() []string { - keys := make([]string, 0, len(labels.values)) - for key := range labels.values { - keys = append(keys, key) - } - - sort.Strings(keys) - - return keys -} - -func (labels *Labels) GetValuesOrderedByKey() []string { - var orderedValues []string - for _, key := range labels.GetKeysInOrder() { - orderedValues = append(orderedValues, labels.values[key]) - } - - return orderedValues -} - -type LabelOption func(*Labels) - -func NewLabels(labelNames []string) *Labels { - labels := &Labels{ - values: make(map[string]string), - } - - for _, label := range labelNames { - labels.values[strings.ToLower(label)] = "" - } - - return labels -} - -func (labels *Labels) WithOptions(options ...LabelOption) { - for _, option := range options { - option(labels) - } -} +const ( + LabelScheme = "scheme" + LabelHost = "host" + LabelPath = "path" + LabelMethod = "method" + LabelStatus = "status" +) -func WithLabel(labelName string, labelValue string) LabelOption { - return func(labels *Labels) { - if _, ok := labels.values[strings.ToLower(labelName)]; !ok { - logrus.Errorf("Attempting to set a value for a label that doesn't exist! '%s' does not exist!", labelName) - } else { - labels.values[strings.ToLower(labelName)] = labelValue - } - } -} +type Labels = prometheus.Labels diff --git a/pkg/metrics/labels_test.go b/pkg/metrics/labels_test.go deleted file mode 100644 index 3347e2ab8a..0000000000 --- a/pkg/metrics/labels_test.go +++ /dev/null @@ -1,191 +0,0 @@ -/* -Copyright 2025 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package metrics - -import ( - "testing" - - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" - "sigs.k8s.io/external-dns/internal/testutils" -) - -func TestNewLabels(t *testing.T) { - tests := []struct { - name string - labelNames []string - expectedLabelNames []string - }{ - { - name: "NewLabels initializes Values with labelNames", - labelNames: []string{"label1", "label2"}, - expectedLabelNames: []string{"label1", "label2"}, - }, - { - name: "NewLabels sets labelNames as lower-case", - labelNames: []string{"LABEL1", "label2"}, - expectedLabelNames: []string{"label1", "label2"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - labels := NewLabels(tt.labelNames) - keys := labels.GetKeysInOrder() - - assert.Equal(t, tt.expectedLabelNames, keys) - }) - } -} - -func TestLabelsWithOptions(t *testing.T) { - tests := []struct { - name string - labelNames []string - options []LabelOption - expectedValuesMap map[string]string - }{ - { - name: "WithOptions sets label values", - labelNames: []string{"label1", "label2"}, - options: []LabelOption{ - WithLabel("label1", "alpha"), - WithLabel("label2", "beta"), - }, - expectedValuesMap: map[string]string{ - "label1": "alpha", - "label2": "beta", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - labels := NewLabels(tt.labelNames) - labels.WithOptions(tt.options...) - - assert.Equal(t, tt.expectedValuesMap, labels.values) - }) - } -} - -func TestLabelsWithLabel(t *testing.T) { - tests := []struct { - name string - labelNames []string - labelName string - labelValue string - expectedLabels *Labels - expectedErrorLog string - }{ - { - name: "WithLabel sets label and value", - labelNames: []string{"label1"}, - labelName: "label1", - labelValue: "alpha", - expectedLabels: &Labels{ - values: map[string]string{ - "label1": "alpha", - }}, - }, - { - name: "WithLabel sets labelName to lowercase", - labelNames: []string{"label1"}, - labelName: "LABEL1", - labelValue: "alpha", - expectedLabels: &Labels{ - values: map[string]string{ - "label1": "alpha", - }}, - }, - { - name: "WithLabel errors if label doesn't exist", - labelNames: []string{"label1"}, - labelName: "notreal", - labelValue: "", - expectedErrorLog: "Attempting to set a value for a label that doesn't exist! 'notreal' does not exist!", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - hook := testutils.LogsUnderTestWithLogLevel(logrus.WarnLevel, t) - - labels := NewLabels(tt.labelNames) - labels.WithOptions(WithLabel(tt.labelName, tt.labelValue)) - - if tt.expectedLabels != nil { - assert.Equal(t, tt.expectedLabels, labels) - } - - if tt.expectedErrorLog != "" { - testutils.TestHelperLogContains(tt.expectedErrorLog, hook, t) - } - }) - } -} - -func TestLabelsGetKeysInOrder(t *testing.T) { - tests := []struct { - name string - labels *Labels - expectedKeysInOrder []string - }{ - { - "GetKeysInOrder returns keys in alphabetical order", - NewLabels([]string{"label2", "label1"}), - []string{"label1", "label2"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - orderedKeys := tt.labels.GetKeysInOrder() - - assert.Equal(t, tt.expectedKeysInOrder, orderedKeys) - }) - } -} - -func TestLabelsGetValuesOrderedByKey(t *testing.T) { - tests := []struct { - name string - labels *Labels - labelOptions []LabelOption - expectedValuesInOrder []string - }{ - { - "GetKeysInOrder returns keys in alphabetical order", - NewLabels([]string{"label1", "label2"}), - []LabelOption{ - WithLabel("label2", "beta"), - WithLabel("label1", "alpha"), - }, - []string{"alpha", "beta"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tt.labels.WithOptions(tt.labelOptions...) - - orderedValues := tt.labels.GetValuesOrderedByKey() - - assert.Equal(t, tt.expectedValuesInOrder, orderedValues) - }) - } -} diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index 477b943c86..3406577cce 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -33,12 +33,6 @@ func (m *MockMetric) Get() *Metric { return &Metric{FQDN: m.FQDN} } -func getTestLabels(t *testing.T, labelNames []string) *Labels { - t.Helper() - - return NewLabels(labelNames) -} - func TestMustRegister(t *testing.T) { tests := []struct { name string @@ -67,7 +61,7 @@ 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"}, *NewLabels([]string{"label"})), + NewSummaryVecWithOpts(prometheus.SummaryOpts{Name: "test_summary_v_3"}, []string{"label"}), }, expected: 5, }, diff --git a/pkg/metrics/models.go b/pkg/metrics/models.go index 134e7dab1a..9998fac16b 100644 --- a/pkg/metrics/models.go +++ b/pkg/metrics/models.go @@ -186,11 +186,11 @@ func (s SummaryVecMetric) Get() *Metric { return &s.Metric } -func (s SummaryVecMetric) SetWithLabels(value float64, labels *Labels) { - s.SummaryVec.WithLabelValues(labels.GetValuesOrderedByKey()...).Observe(value) +func (s SummaryVecMetric) SetWithLabels(value float64, labels prometheus.Labels) { + s.SummaryVec.With(labels).Observe(value) } -func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labels Labels) SummaryVecMetric { +func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labels []string) SummaryVecMetric { opts.Namespace = Namespace return SummaryVecMetric{ Metric: Metric{ @@ -201,7 +201,7 @@ func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labels Labels) SummaryVe Subsystem: opts.Subsystem, Help: opts.Help, }, - SummaryVec: *prometheus.NewSummaryVec(opts, labels.GetKeysInOrder()), + SummaryVec: *prometheus.NewSummaryVec(opts, labels), } } diff --git a/pkg/metrics/models_test.go b/pkg/metrics/models_test.go index ec49699997..8aa67b4edb 100644 --- a/pkg/metrics/models_test.go +++ b/pkg/metrics/models_test.go @@ -192,13 +192,11 @@ func TestSummaryV_SetWithLabels(t *testing.T) { Help: "help text", } - labels := NewLabels([]string{"label1", "label2"}) - sv := NewSummaryVecWithOpts(opts, *labels) + labels := Labels{} + sv := NewSummaryVecWithOpts(opts, []string{"label1", "label2"}) - labels.WithOptions( - WithLabel("label1", "alpha"), - WithLabel("label2", "beta"), - ) + labels["label1"] = "alpha" + labels["label2"] = "beta" sv.SetWithLabels(5.01, labels) diff --git a/provider/aws/instrumented_config.go b/provider/aws/instrumented_config.go index 91d2b6e755..8dceed6ae9 100644 --- a/provider/aws/instrumented_config.go +++ b/provider/aws/instrumented_config.go @@ -61,23 +61,22 @@ var extractAWSRequestParameters = middleware.DeserializeMiddlewareFunc("extractA requestMetrics := getRequestMetric(ctx) + labels := metrics.Labels{} + if req, ok := in.Request.(*smithyhttp.Request); ok && req != nil { - extdnshttp.RequestDurationLabels.WithOptions( - metrics.WithLabel("scheme", req.URL.Scheme), - metrics.WithLabel("host", req.URL.Host), - metrics.WithLabel("path", metrics.PathProcessor(req.URL.Path)), - metrics.WithLabel("method", req.Method), - ) + labels[metrics.LabelScheme] = req.URL.Scheme + labels[metrics.LabelHost] = req.URL.Host + labels[metrics.LabelPath] = metrics.PathProcessor(req.URL.Path) + labels[metrics.LabelMethod] = req.Method + labels[metrics.LabelStatus] = "unknown" } // Try to access HTTP response and status code if resp, ok := out.RawResponse.(*smithyhttp.Response); ok && resp != nil { - extdnshttp.RequestDurationLabels.WithOptions( - metrics.WithLabel("status", fmt.Sprintf("%d", resp.StatusCode)), - ) + labels[metrics.LabelStatus] = fmt.Sprintf("%d", resp.StatusCode) } - extdnshttp.RequestDurationMetric.SetWithLabels(time.Since(requestMetrics.StartTime).Seconds(), extdnshttp.RequestDurationLabels) + extdnshttp.RequestDurationMetric.SetWithLabels(time.Since(requestMetrics.StartTime).Seconds(), labels) return out, metadata, err })