Skip to content

Commit 32ec8d4

Browse files
rexagodrexagod
committed
fixup! fixup! fixup! address metrics.(*Counter) context data-race
1 parent fd3747f commit 32ec8d4

File tree

2 files changed

+91
-105
lines changed

2 files changed

+91
-105
lines changed

‎staging/src/k8s.io/component-base/metrics/counter.go

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -295,26 +295,7 @@ func (v *CounterVec) ResetLabelAllowLists() {
295295
v.LabelValueAllowLists = nil
296296
}
297297

298-
// WithContext returns wrapped CounterVec with context
299-
func (v *CounterVec) WithContext(ctx context.Context) *CounterVecWithContext {
300-
return &CounterVecWithContext{
301-
ctx: ctx,
302-
CounterVec: v,
303-
}
304-
}
305-
306-
// CounterVecWithContext is the wrapper of CounterVec with context.
307-
type CounterVecWithContext struct {
308-
*CounterVec
309-
ctx context.Context
310-
}
311-
312-
// WithLabelValues is the wrapper of CounterVec.WithLabelValues.
313-
func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) CounterMetric {
314-
return vc.CounterVec.WithLabelValues(lvs...)
315-
}
316-
317-
// With is the wrapper of CounterVec.With.
318-
func (vc *CounterVecWithContext) With(labels map[string]string) CounterMetric {
319-
return vc.CounterVec.With(labels)
298+
// WithContext is a no-op.
299+
func (v *CounterVec) WithContext(_ context.Context) *CounterVec {
300+
return v
320301
}

‎staging/src/k8s.io/component-base/metrics/counter_test.go

Lines changed: 88 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,13 @@ func TestCounterWithExemplar(t *testing.T) {
300300
}
301301
return arr
302302
}
303+
originalTraceID := trace.TraceID(fn(0))
304+
originalSpanID := trace.SpanID(fn(1))
305+
redundantCtxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{
306+
SpanID: originalSpanID,
307+
TraceID: originalTraceID,
308+
TraceFlags: trace.FlagsSampled,
309+
}))
303310
traceID := trace.TraceID(fn(1))
304311
spanID := trace.SpanID(fn(2))
305312
ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{
@@ -323,63 +330,30 @@ func TestCounterWithExemplar(t *testing.T) {
323330
})
324331
registry.MustRegister(counter)
325332

326-
// Call underlying exemplar methods.
327-
counter.Add(toAdd)
328-
counter.Inc()
329-
counter.Inc()
333+
// Call underlying exemplar methods, overriding the original span context.
334+
counter.WithContext(redundantCtxForSpanCtx).Add(toAdd) // This should be counted, but it's context should be overridden.
335+
counter.WithContext(redundantCtxForSpanCtx).Inc() // This should be counted, but it's context should be overridden.
336+
counter.Inc() // This should be counted, and it's (parent's) context should be used.
330337

331-
// Gather.
332-
mfs, err := registry.Gather()
333-
if err != nil {
334-
t.Fatalf("Gather failed %v", err)
335-
}
336-
if len(mfs) != 1 {
337-
t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs))
338-
}
338+
// Verify exemplar with the overridden span context.
339+
exemplarGatherAndVerify(t, registry, traceID, spanID, 1, toAdd+2)
339340

340-
// Verify metric type.
341-
mf := mfs[0]
342-
var m *dto.Metric
343-
switch mf.GetType() {
344-
case dto.MetricType_COUNTER:
345-
m = mfs[0].GetMetric()[0]
346-
default:
347-
t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER)
348-
}
341+
// Modify span context.
342+
altTraceID := trace.TraceID(fn(2))
343+
altSpanID := trace.SpanID(fn(3))
344+
altCtxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{
345+
SpanID: altSpanID,
346+
TraceID: altTraceID,
347+
TraceFlags: trace.FlagsSampled,
348+
}))
349349

350-
// Verify value.
351-
want := toAdd + 2
352-
got := m.GetCounter().GetValue()
353-
if got != want {
354-
t.Fatalf("Got %f, wanted %f as the count", got, want)
355-
}
350+
// Call underlying exemplar methods with different span context.
351+
counter.WithContext(ctxForSpanCtx).Add(toAdd) // This should be counted, but it's context should be overridden.
352+
counter.WithContext(ctxForSpanCtx).Inc() // This should be counted, but it's context should be overridden.
353+
counter.WithContext(altCtxForSpanCtx).Inc() // This should be counted, and it's context should be used.
356354

357-
// Verify exemplars.
358-
e := m.GetCounter().GetExemplar()
359-
if e == nil {
360-
t.Fatalf("Got nil exemplar, wanted an exemplar")
361-
}
362-
eLabels := e.GetLabel()
363-
if eLabels == nil {
364-
t.Fatalf("Got nil exemplar label, wanted an exemplar label")
365-
}
366-
if len(eLabels) != 2 {
367-
t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels))
368-
}
369-
for _, l := range eLabels {
370-
switch *l.Name {
371-
case "trace_id":
372-
if *l.Value != traceID.String() {
373-
t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String())
374-
}
375-
case "span_id":
376-
if *l.Value != spanID.String() {
377-
t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String())
378-
}
379-
default:
380-
t.Fatalf("Got unexpected label %s", *l.Name)
381-
}
382-
}
355+
// Verify exemplar with different span context.
356+
exemplarGatherAndVerify(t, registry, altTraceID, altSpanID, 1, 2*toAdd+4)
383357

384358
// Verify that all contextual counter calls are exclusive.
385359
contextualCounter := NewCounter(&CounterOpts{
@@ -421,41 +395,72 @@ func TestCounterWithExemplar(t *testing.T) {
421395
contextualCounter: contextualCounterB,
422396
},
423397
}
424-
for _, run := range runs {
398+
for i, run := range runs {
425399
registry.MustRegister(run.contextualCounter)
426400
run.contextualCounter.Inc()
401+
exemplarGatherAndVerify(t, registry, run.traceID, run.spanID, 2, float64(i+1))
402+
registry.Unregister(run.contextualCounter)
403+
}
404+
}
427405

428-
mfs, err = registry.Gather()
429-
if err != nil {
430-
t.Fatalf("Gather failed %v", err)
431-
}
432-
if len(mfs) != 2 {
433-
t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs))
434-
}
406+
func exemplarGatherAndVerify(t *testing.T,
407+
registry *kubeRegistry,
408+
traceID trace.TraceID,
409+
spanID trace.SpanID,
410+
expectedMetricFamilies int,
411+
expectedValue float64,
412+
) {
435413

436-
dtoMetric := mfs[0].GetMetric()[0]
437-
if dtoMetric.GetCounter().GetExemplar() == nil {
438-
t.Fatalf("Got nil exemplar, wanted an exemplar")
439-
}
440-
dtoMetricLabels := dtoMetric.GetCounter().GetExemplar().GetLabel()
441-
if len(dtoMetricLabels) != 2 {
442-
t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels))
443-
}
444-
for _, l := range dtoMetricLabels {
445-
switch *l.Name {
446-
case "trace_id":
447-
if *l.Value != run.traceID.String() {
448-
t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String())
449-
}
450-
case "span_id":
451-
if *l.Value != run.spanID.String() {
452-
t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String())
453-
}
454-
default:
455-
t.Fatalf("Got unexpected label %s", *l.Name)
414+
// Gather.
415+
mfs, err := registry.Gather()
416+
if err != nil {
417+
t.Fatalf("Gather failed %v", err)
418+
}
419+
if len(mfs) != expectedMetricFamilies {
420+
t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs))
421+
}
422+
423+
// Verify metric type.
424+
mf := mfs[0]
425+
var m *dto.Metric
426+
switch mf.GetType() {
427+
case dto.MetricType_COUNTER:
428+
m = mfs[0].GetMetric()[0]
429+
default:
430+
t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER)
431+
}
432+
433+
// Verify value.
434+
want := expectedValue
435+
got := m.GetCounter().GetValue()
436+
if got != want {
437+
t.Fatalf("Got %f, wanted %f as the count", got, want)
438+
}
439+
440+
// Verify exemplars.
441+
e := m.GetCounter().GetExemplar()
442+
if e == nil {
443+
t.Fatalf("Got nil exemplar, wanted an exemplar")
444+
}
445+
eLabels := e.GetLabel()
446+
if eLabels == nil {
447+
t.Fatalf("Got nil exemplar label, wanted an exemplar label")
448+
}
449+
if len(eLabels) != 2 {
450+
t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels))
451+
}
452+
for _, l := range eLabels {
453+
switch *l.Name {
454+
case "trace_id":
455+
if *l.Value != traceID.String() {
456+
t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String())
456457
}
458+
case "span_id":
459+
if *l.Value != spanID.String() {
460+
t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String())
461+
}
462+
default:
463+
t.Fatalf("Got unexpected label %s", *l.Name)
457464
}
458-
459-
registry.Unregister(run.contextualCounter)
460465
}
461466
}

0 commit comments

Comments
 (0)