Skip to content

Commit 8ef5580

Browse files
mwmixmwmix
authored
fix(metrics): Make prometheus metrics labels more type safe. (#5717)
1 parent 4a27943 commit 8ef5580

File tree

6 files changed

+301
-15
lines changed

6 files changed

+301
-15
lines changed

‎pkg/http/http.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
)
3030

3131
var (
32+
RequestDurationLabels = metrics.NewLabels([]string{"scheme", "host", "path", "method", "status"})
3233
RequestDurationMetric = metrics.NewSummaryVecWithOpts(
3334
prometheus.SummaryOpts{
3435
Name: "request_duration_seconds",
@@ -37,7 +38,7 @@ var (
3738
ConstLabels: prometheus.Labels{"handler": "instrumented_http"},
3839
Objectives: map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001},
3940
},
40-
[]string{"scheme", "host", "path", "method", "status"},
41+
*RequestDurationLabels,
4142
)
4243
)
4344

@@ -62,7 +63,16 @@ func (r *CustomRoundTripper) RoundTrip(req *http.Request) (*http.Response, error
6263
if resp != nil {
6364
status = fmt.Sprintf("%d", resp.StatusCode)
6465
}
65-
RequestDurationMetric.SetWithLabels(time.Since(start).Seconds(), req.URL.Scheme, req.URL.Host, metrics.PathProcessor(req.URL.Path), req.Method, status)
66+
67+
RequestDurationLabels.WithOptions(
68+
metrics.WithLabel("scheme", req.URL.Scheme),
69+
metrics.WithLabel("host", req.URL.Host),
70+
metrics.WithLabel("path", metrics.PathProcessor(req.URL.Path)),
71+
metrics.WithLabel("method", req.Method),
72+
metrics.WithLabel("status", status),
73+
)
74+
75+
RequestDurationMetric.SetWithLabels(time.Since(start).Seconds(), RequestDurationLabels)
6676

6777
return resp, err
6878
}

‎pkg/metrics/labels.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package metrics
18+
19+
import (
20+
"sort"
21+
"strings"
22+
23+
"github.com/sirupsen/logrus"
24+
)
25+
26+
type Labels struct {
27+
values map[string]string
28+
}
29+
30+
func (labels *Labels) GetKeysInOrder() []string {
31+
keys := make([]string, 0, len(labels.values))
32+
for key := range labels.values {
33+
keys = append(keys, key)
34+
}
35+
36+
sort.Strings(keys)
37+
38+
return keys
39+
}
40+
41+
func (labels *Labels) GetValuesOrderedByKey() []string {
42+
var orderedValues []string
43+
for _, key := range labels.GetKeysInOrder() {
44+
orderedValues = append(orderedValues, labels.values[key])
45+
}
46+
47+
return orderedValues
48+
}
49+
50+
type LabelOption func(*Labels)
51+
52+
func NewLabels(labelNames []string) *Labels {
53+
labels := &Labels{
54+
values: make(map[string]string),
55+
}
56+
57+
for _, label := range labelNames {
58+
labels.values[strings.ToLower(label)] = ""
59+
}
60+
61+
return labels
62+
}
63+
64+
func (labels *Labels) WithOptions(options ...LabelOption) {
65+
for _, option := range options {
66+
option(labels)
67+
}
68+
}
69+
70+
func WithLabel(labelName string, labelValue string) LabelOption {
71+
return func(labels *Labels) {
72+
if _, ok := labels.values[strings.ToLower(labelName)]; !ok {
73+
logrus.Errorf("Attempting to set a value for a label that doesn't exist! '%s' does not exist!", labelName)
74+
} else {
75+
labels.values[strings.ToLower(labelName)] = labelValue
76+
}
77+
}
78+
}

‎pkg/metrics/labels_test.go‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package metrics
18+
19+
import (
20+
"testing"
21+
22+
"github.com/sirupsen/logrus"
23+
"github.com/stretchr/testify/assert"
24+
"sigs.k8s.io/external-dns/internal/testutils"
25+
)
26+
27+
func TestNewLabels(t *testing.T) {
28+
tests := []struct {
29+
name string
30+
labelNames []string
31+
expectedLabelNames []string
32+
}{
33+
{
34+
name: "NewLabels initializes Values with labelNames",
35+
labelNames: []string{"label1", "label2"},
36+
expectedLabelNames: []string{"label1", "label2"},
37+
},
38+
{
39+
name: "NewLabels sets labelNames as lower-case",
40+
labelNames: []string{"LABEL1", "label2"},
41+
expectedLabelNames: []string{"label1", "label2"},
42+
},
43+
}
44+
45+
for _, tt := range tests {
46+
t.Run(tt.name, func(t *testing.T) {
47+
labels := NewLabels(tt.labelNames)
48+
keys := labels.GetKeysInOrder()
49+
50+
assert.Equal(t, tt.expectedLabelNames, keys)
51+
})
52+
}
53+
}
54+
55+
func TestLabelsWithOptions(t *testing.T) {
56+
tests := []struct {
57+
name string
58+
labelNames []string
59+
options []LabelOption
60+
expectedValuesMap map[string]string
61+
}{
62+
{
63+
name: "WithOptions sets label values",
64+
labelNames: []string{"label1", "label2"},
65+
options: []LabelOption{
66+
WithLabel("label1", "alpha"),
67+
WithLabel("label2", "beta"),
68+
},
69+
expectedValuesMap: map[string]string{
70+
"label1": "alpha",
71+
"label2": "beta",
72+
},
73+
},
74+
}
75+
76+
for _, tt := range tests {
77+
t.Run(tt.name, func(t *testing.T) {
78+
labels := NewLabels(tt.labelNames)
79+
labels.WithOptions(tt.options...)
80+
81+
assert.Equal(t, tt.expectedValuesMap, labels.values)
82+
})
83+
}
84+
}
85+
86+
func TestLabelsWithLabel(t *testing.T) {
87+
tests := []struct {
88+
name string
89+
labelNames []string
90+
labelName string
91+
labelValue string
92+
expectedLabels *Labels
93+
expectedErrorLog string
94+
}{
95+
{
96+
name: "WithLabel sets label and value",
97+
labelNames: []string{"label1"},
98+
labelName: "label1",
99+
labelValue: "alpha",
100+
expectedLabels: &Labels{
101+
values: map[string]string{
102+
"label1": "alpha",
103+
}},
104+
},
105+
{
106+
name: "WithLabel sets labelName to lowercase",
107+
labelNames: []string{"label1"},
108+
labelName: "LABEL1",
109+
labelValue: "alpha",
110+
expectedLabels: &Labels{
111+
values: map[string]string{
112+
"label1": "alpha",
113+
}},
114+
},
115+
{
116+
name: "WithLabel errors if label doesn't exist",
117+
labelNames: []string{"label1"},
118+
labelName: "notreal",
119+
labelValue: "",
120+
expectedErrorLog: "Attempting to set a value for a label that doesn't exist! 'notreal' does not exist!",
121+
},
122+
}
123+
124+
for _, tt := range tests {
125+
t.Run(tt.name, func(t *testing.T) {
126+
hook := testutils.LogsUnderTestWithLogLevel(logrus.WarnLevel, t)
127+
128+
labels := NewLabels(tt.labelNames)
129+
labels.WithOptions(WithLabel(tt.labelName, tt.labelValue))
130+
131+
if tt.expectedLabels != nil {
132+
assert.Equal(t, tt.expectedLabels, labels)
133+
}
134+
135+
if tt.expectedErrorLog != "" {
136+
testutils.TestHelperLogContains(tt.expectedErrorLog, hook, t)
137+
}
138+
})
139+
}
140+
}
141+
142+
func TestLabelsGetKeysInOrder(t *testing.T) {
143+
tests := []struct {
144+
name string
145+
labels *Labels
146+
expectedKeysInOrder []string
147+
}{
148+
{
149+
"GetKeysInOrder returns keys in alphabetical order",
150+
NewLabels([]string{"label2", "label1"}),
151+
[]string{"label1", "label2"},
152+
},
153+
}
154+
155+
for _, tt := range tests {
156+
t.Run(tt.name, func(t *testing.T) {
157+
orderedKeys := tt.labels.GetKeysInOrder()
158+
159+
assert.Equal(t, tt.expectedKeysInOrder, orderedKeys)
160+
})
161+
}
162+
}
163+
164+
func TestLabelsGetValuesOrderedByKey(t *testing.T) {
165+
tests := []struct {
166+
name string
167+
labels *Labels
168+
labelOptions []LabelOption
169+
expectedValuesInOrder []string
170+
}{
171+
{
172+
"GetKeysInOrder returns keys in alphabetical order",
173+
NewLabels([]string{"label1", "label2"}),
174+
[]LabelOption{
175+
WithLabel("label2", "beta"),
176+
WithLabel("label1", "alpha"),
177+
},
178+
[]string{"alpha", "beta"},
179+
},
180+
}
181+
182+
for _, tt := range tests {
183+
t.Run(tt.name, func(t *testing.T) {
184+
tt.labels.WithOptions(tt.labelOptions...)
185+
186+
orderedValues := tt.labels.GetValuesOrderedByKey()
187+
188+
assert.Equal(t, tt.expectedValuesInOrder, orderedValues)
189+
})
190+
}
191+
}

‎pkg/metrics/metrics_test.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ func (m *MockMetric) Get() *Metric {
3333
return &Metric{FQDN: m.FQDN}
3434
}
3535

36+
func getTestLabels(t *testing.T, labelNames []string) *Labels {
37+
t.Helper()
38+
39+
return NewLabels(labelNames)
40+
}
41+
3642
func TestMustRegister(t *testing.T) {
3743
tests := []struct {
3844
name string
@@ -61,7 +67,7 @@ func TestMustRegister(t *testing.T) {
6167
NewCounterWithOpts(prometheus.CounterOpts{Name: "test_counter_3"}),
6268
NewCounterVecWithOpts(prometheus.CounterOpts{Name: "test_counter_vec_3"}, []string{"label"}),
6369
NewGaugedVectorOpts(prometheus.GaugeOpts{Name: "test_gauge_v_3"}, []string{"label"}),
64-
NewSummaryVecWithOpts(prometheus.SummaryOpts{Name: "test_summary_v_3"}, []string{"label"}),
70+
NewSummaryVecWithOpts(prometheus.SummaryOpts{Name: "test_summary_v_3"}, *NewLabels([]string{"label"})),
6571
},
6672
expected: 5,
6773
},

‎pkg/metrics/models.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -186,17 +186,11 @@ func (s SummaryVecMetric) Get() *Metric {
186186
return &s.Metric
187187
}
188188

189-
// SetWithLabels sets the value of the SummaryVec metric for the specified label values.
190-
// All label values are converted to lowercase before being applied.
191-
func (s SummaryVecMetric) SetWithLabels(value float64, lvs ...string) {
192-
for i, v := range lvs {
193-
lvs[i] = strings.ToLower(v)
194-
}
195-
196-
s.SummaryVec.WithLabelValues(lvs...).Observe(value)
189+
func (s SummaryVecMetric) SetWithLabels(value float64, labels *Labels) {
190+
s.SummaryVec.WithLabelValues(labels.GetValuesOrderedByKey()...).Observe(value)
197191
}
198192

199-
func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labelNames []string) SummaryVecMetric {
193+
func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labels Labels) SummaryVecMetric {
200194
opts.Namespace = Namespace
201195
return SummaryVecMetric{
202196
Metric: Metric{
@@ -207,7 +201,7 @@ func NewSummaryVecWithOpts(opts prometheus.SummaryOpts, labelNames []string) Sum
207201
Subsystem: opts.Subsystem,
208202
Help: opts.Help,
209203
},
210-
SummaryVec: *prometheus.NewSummaryVec(opts, labelNames),
204+
SummaryVec: *prometheus.NewSummaryVec(opts, labels.GetKeysInOrder()),
211205
}
212206
}
213207

‎pkg/metrics/models_test.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,16 @@ func TestSummaryV_SetWithLabels(t *testing.T) {
191191
Subsystem: "test_sub",
192192
Help: "help text",
193193
}
194-
sv := NewSummaryVecWithOpts(opts, []string{"label1", "label2"})
195194

196-
sv.SetWithLabels(5.01, "Alpha", "BETA")
195+
labels := NewLabels([]string{"label1", "label2"})
196+
sv := NewSummaryVecWithOpts(opts, *labels)
197+
198+
labels.WithOptions(
199+
WithLabel("label1", "alpha"),
200+
WithLabel("label2", "beta"),
201+
)
202+
203+
sv.SetWithLabels(5.01, labels)
197204

198205
reg := prometheus.NewRegistry()
199206
reg.MustRegister(sv.SummaryVec)

0 commit comments

Comments
 (0)