Skip to content

Commit c136bbe

Browse files
spikecurtisspikecurtis
committed
fix: fix MetricsAggregator check for metric sameness
1 parent fb29af6 commit c136bbe

File tree

3 files changed

+252
-2
lines changed

3 files changed

+252
-2
lines changed

‎coderd/prometheusmetrics/aggregator.go

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"time"
66

77
"github.com/prometheus/client_golang/prometheus"
8-
"golang.org/x/exp/slices"
98
"golang.org/x/xerrors"
109

1110
"cdr.dev/slog"
@@ -68,7 +67,32 @@ type annotatedMetric struct {
6867
var _ prometheus.Collector = new(MetricsAggregator)
6968

7069
func (am *annotatedMetric) is(req updateRequest, m *agentproto.Stats_Metric) bool {
71-
return am.username == req.username && am.workspaceName == req.workspaceName && am.agentName == req.agentName && am.Name == m.Name && slices.Equal(am.Labels, m.Labels)
70+
return am.username == req.username &&
71+
am.workspaceName == req.workspaceName &&
72+
am.agentName == req.agentName &&
73+
am.templateName == req.templateName &&
74+
am.Name == m.Name &&
75+
labelsEqual(am.Labels, m.Labels)
76+
}
77+
78+
func labelsEqual(a, b []*agentproto.Stats_Metric_Label) bool {
79+
if len(a) != len(b) {
80+
return false
81+
}
82+
am := make(map[string]string)
83+
for _, l := range a {
84+
am[l.GetName()] = l.GetValue()
85+
}
86+
for _, l := range b {
87+
v, ok := am[l.GetName()]
88+
if !ok {
89+
return false
90+
}
91+
if v != l.GetValue() {
92+
return false
93+
}
94+
}
95+
return true
7296
}
7397

7498
func (am *annotatedMetric) asPrometheus() (prometheus.Metric, error) {

‎coderd/prometheusmetrics/aggregator_internal_test.go

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
package prometheusmetrics
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
"github.com/coder/coder/v2/agent/proto"
10+
)
11+
12+
func TestAnnotedMetric_Is(t *testing.T) {
13+
t.Parallel()
14+
am1 := &annotatedMetric{
15+
Stats_Metric: &proto.Stats_Metric{
16+
Name: "met",
17+
Type: proto.Stats_Metric_COUNTER,
18+
Value: 1,
19+
Labels: []*proto.Stats_Metric_Label{
20+
{Name: "rarity", Value: "blue moon"},
21+
{Name: "certainty", Value: "yes"},
22+
},
23+
},
24+
username: "spike",
25+
workspaceName: "work",
26+
agentName: "janus",
27+
templateName: "tempe",
28+
expiryDate: time.Now(),
29+
}
30+
for _, tc := range []struct {
31+
name string
32+
req updateRequest
33+
m *proto.Stats_Metric
34+
is bool
35+
}{
36+
{
37+
name: "OK",
38+
req: updateRequest{
39+
username: "spike",
40+
workspaceName: "work",
41+
agentName: "janus",
42+
templateName: "tempe",
43+
metrics: nil,
44+
timestamp: time.Now().Add(-5 * time.Second),
45+
},
46+
m: &proto.Stats_Metric{
47+
Name: "met",
48+
Type: proto.Stats_Metric_COUNTER,
49+
Value: 2,
50+
Labels: []*proto.Stats_Metric_Label{
51+
{Name: "rarity", Value: "blue moon"},
52+
{Name: "certainty", Value: "yes"},
53+
},
54+
},
55+
is: true,
56+
},
57+
{
58+
name: "missingLabel",
59+
req: updateRequest{
60+
username: "spike",
61+
workspaceName: "work",
62+
agentName: "janus",
63+
templateName: "tempe",
64+
metrics: nil,
65+
timestamp: time.Now().Add(-5 * time.Second),
66+
},
67+
m: &proto.Stats_Metric{
68+
Name: "met",
69+
Type: proto.Stats_Metric_COUNTER,
70+
Value: 2,
71+
Labels: []*proto.Stats_Metric_Label{
72+
{Name: "certainty", Value: "yes"},
73+
},
74+
},
75+
is: false,
76+
},
77+
{
78+
name: "wrongLabelValue",
79+
req: updateRequest{
80+
username: "spike",
81+
workspaceName: "work",
82+
agentName: "janus",
83+
templateName: "tempe",
84+
metrics: nil,
85+
timestamp: time.Now().Add(-5 * time.Second),
86+
},
87+
m: &proto.Stats_Metric{
88+
Name: "met",
89+
Type: proto.Stats_Metric_COUNTER,
90+
Value: 2,
91+
Labels: []*proto.Stats_Metric_Label{
92+
{Name: "rarity", Value: "blue moon"},
93+
{Name: "certainty", Value: "inshallah"},
94+
},
95+
},
96+
is: false,
97+
},
98+
{
99+
name: "wrongMetricName",
100+
req: updateRequest{
101+
username: "spike",
102+
workspaceName: "work",
103+
agentName: "janus",
104+
templateName: "tempe",
105+
metrics: nil,
106+
timestamp: time.Now().Add(-5 * time.Second),
107+
},
108+
m: &proto.Stats_Metric{
109+
Name: "cub",
110+
Type: proto.Stats_Metric_COUNTER,
111+
Value: 2,
112+
Labels: []*proto.Stats_Metric_Label{
113+
{Name: "rarity", Value: "blue moon"},
114+
{Name: "certainty", Value: "yes"},
115+
},
116+
},
117+
is: false,
118+
},
119+
{
120+
name: "wrongUsername",
121+
req: updateRequest{
122+
username: "steve",
123+
workspaceName: "work",
124+
agentName: "janus",
125+
templateName: "tempe",
126+
metrics: nil,
127+
timestamp: time.Now().Add(-5 * time.Second),
128+
},
129+
m: &proto.Stats_Metric{
130+
Name: "met",
131+
Type: proto.Stats_Metric_COUNTER,
132+
Value: 2,
133+
Labels: []*proto.Stats_Metric_Label{
134+
{Name: "rarity", Value: "blue moon"},
135+
{Name: "certainty", Value: "yes"},
136+
},
137+
},
138+
is: false,
139+
},
140+
{
141+
name: "wrongWorkspaceName",
142+
req: updateRequest{
143+
username: "spike",
144+
workspaceName: "play",
145+
agentName: "janus",
146+
templateName: "tempe",
147+
metrics: nil,
148+
timestamp: time.Now().Add(-5 * time.Second),
149+
},
150+
m: &proto.Stats_Metric{
151+
Name: "met",
152+
Type: proto.Stats_Metric_COUNTER,
153+
Value: 2,
154+
Labels: []*proto.Stats_Metric_Label{
155+
{Name: "rarity", Value: "blue moon"},
156+
{Name: "certainty", Value: "yes"},
157+
},
158+
},
159+
is: false,
160+
},
161+
{
162+
name: "wrongAgentName",
163+
req: updateRequest{
164+
username: "spike",
165+
workspaceName: "work",
166+
agentName: "bond",
167+
templateName: "tempe",
168+
metrics: nil,
169+
timestamp: time.Now().Add(-5 * time.Second),
170+
},
171+
m: &proto.Stats_Metric{
172+
Name: "met",
173+
Type: proto.Stats_Metric_COUNTER,
174+
Value: 2,
175+
Labels: []*proto.Stats_Metric_Label{
176+
{Name: "rarity", Value: "blue moon"},
177+
{Name: "certainty", Value: "yes"},
178+
},
179+
},
180+
is: false,
181+
},
182+
{
183+
name: "wrongTemplateName",
184+
req: updateRequest{
185+
username: "spike",
186+
workspaceName: "work",
187+
agentName: "janus",
188+
templateName: "phoenix",
189+
metrics: nil,
190+
timestamp: time.Now().Add(-5 * time.Second),
191+
},
192+
m: &proto.Stats_Metric{
193+
Name: "met",
194+
Type: proto.Stats_Metric_COUNTER,
195+
Value: 2,
196+
Labels: []*proto.Stats_Metric_Label{
197+
{Name: "rarity", Value: "blue moon"},
198+
{Name: "certainty", Value: "yes"},
199+
},
200+
},
201+
is: false,
202+
},
203+
} {
204+
tc := tc
205+
t.Run(tc.name, func(t *testing.T) {
206+
t.Parallel()
207+
require.Equal(t, tc.is, am1.is(tc.req, tc.m))
208+
})
209+
}
210+
}

‎coderd/prometheusmetrics/aggregator_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,19 @@ func TestUpdateMetrics_MetricsDoNotExpire(t *testing.T) {
5151
given1 := []*agentproto.Stats_Metric{
5252
{Name: "a_counter_one", Type: agentproto.Stats_Metric_COUNTER, Value: 1},
5353
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: 2},
54+
// Tests that we update labels correctly when they have extra labels
55+
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: 27, Labels: []*agentproto.Stats_Metric_Label{
56+
{Name: "lizz", Value: "rizz"},
57+
}},
5458
{Name: "c_gauge_three", Type: agentproto.Stats_Metric_GAUGE, Value: 3},
5559
}
5660

5761
given2 := []*agentproto.Stats_Metric{
5862
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: 4},
63+
// Tests that we update labels correctly when they have extra labels
64+
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: -9, Labels: []*agentproto.Stats_Metric_Label{
65+
{Name: "lizz", Value: "rizz"},
66+
}},
5967
{Name: "c_gauge_three", Type: agentproto.Stats_Metric_GAUGE, Value: 5},
6068
{Name: "c_gauge_three", Type: agentproto.Stats_Metric_GAUGE, Value: 2, Labels: []*agentproto.Stats_Metric_Label{
6169
{Name: "foobar", Value: "Foobaz"},
@@ -73,6 +81,13 @@ func TestUpdateMetrics_MetricsDoNotExpire(t *testing.T) {
7381
expected := []*agentproto.Stats_Metric{
7482
{Name: "a_counter_one", Type: agentproto.Stats_Metric_COUNTER, Value: 1, Labels: commonLabels},
7583
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: 4, Labels: commonLabels},
84+
{Name: "b_counter_two", Type: agentproto.Stats_Metric_COUNTER, Value: -9, Labels: []*agentproto.Stats_Metric_Label{
85+
{Name: "agent_name", Value: testAgentName},
86+
{Name: "lizz", Value: "rizz"},
87+
{Name: "username", Value: testUsername},
88+
{Name: "workspace_name", Value: testWorkspaceName},
89+
{Name: "template_name", Value: testTemplateName},
90+
}},
7691
{Name: "c_gauge_three", Type: agentproto.Stats_Metric_GAUGE, Value: 5, Labels: commonLabels},
7792
{Name: "c_gauge_three", Type: agentproto.Stats_Metric_GAUGE, Value: 2, Labels: []*agentproto.Stats_Metric_Label{
7893
{Name: "agent_name", Value: testAgentName},
@@ -111,6 +126,7 @@ func TestUpdateMetrics_MetricsDoNotExpire(t *testing.T) {
111126

112127
func verifyCollectedMetrics(t *testing.T, expected []*agentproto.Stats_Metric, actual []prometheus.Metric) bool {
113128
if len(expected) != len(actual) {
129+
t.Logf("expected %d metrics, got %d", len(expected), len(actual))
114130
return false
115131
}
116132

0 commit comments

Comments
 (0)