From e85cd374036e0009495e35ceb9d60ee04b7819e0 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Tue, 5 Nov 2024 18:37:40 +0530 Subject: [PATCH 1/5] address `metrics.(*Counter)` context data-race Fixes: https://github.com/kubernetes/kubernetes/issues/128548 Signed-off-by: Pranshu Srivastava --- .../k8s.io/component-base/metrics/counter.go | 105 +++++--- .../component-base/metrics/counter_test.go | 254 +++++++++++++++++- .../component-base/metrics/histogram.go | 44 +-- .../component-base/metrics/histogram_test.go | 207 ++++++++++++-- 4 files changed, 537 insertions(+), 73 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index e41d5383bee64..31f113603e191 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -30,23 +30,22 @@ import ( // Counter is our internal representation for our wrapping struct around prometheus // counters. Counter implements both kubeCollector and CounterMetric. type Counter struct { - ctx context.Context CounterMetric *CounterOpts lazyMetric selfCollector } +type CounterWithContext struct { + ctx context.Context + *Counter +} + // The implementation of the Metric interface is expected by testutil.GetCounterMetricValue. var _ Metric = &Counter{} // All supported exemplar metric types implement the metricWithExemplar interface. -var _ metricWithExemplar = &Counter{} - -// exemplarCounterMetric holds a context to extract exemplar labels from, and a counter metric to attach them to. It implements the metricWithExemplar interface. -type exemplarCounterMetric struct { - *Counter -} +var _ metricWithExemplar = &CounterWithContext{} // NewCounter returns an object which satisfies the kubeCollector and CounterMetric interfaces. // However, the object returned will not measure anything unless the collector is first @@ -106,28 +105,17 @@ func (c *Counter) initializeDeprecatedMetric() { } // WithContext allows the normal Counter metric to pass in context. -func (c *Counter) WithContext(ctx context.Context) CounterMetric { - c.ctx = ctx - return c.CounterMetric -} - -// withExemplar initializes the exemplarMetric object and sets the exemplar value. -func (c *Counter) withExemplar(v float64) { - (&exemplarCounterMetric{c}).withExemplar(v) -} - -func (c *Counter) Add(v float64) { - c.withExemplar(v) -} - -func (c *Counter) Inc() { - c.withExemplar(1) +func (c *Counter) WithContext(ctx context.Context) *CounterWithContext { + return &CounterWithContext{ + ctx: ctx, + Counter: c, + } } // withExemplar attaches an exemplar to the metric. -func (e *exemplarCounterMetric) withExemplar(v float64) { - if m, ok := e.CounterMetric.(prometheus.ExemplarAdder); ok { - maybeSpanCtx := trace.SpanContextFromContext(e.ctx) +func (c *CounterWithContext) withExemplar(v float64) { + if m, ok := c.CounterMetric.(prometheus.ExemplarAdder); ok { + maybeSpanCtx := trace.SpanContextFromContext(c.ctx) if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { exemplarLabels := prometheus.Labels{ "trace_id": maybeSpanCtx.TraceID().String(), @@ -138,7 +126,23 @@ func (e *exemplarCounterMetric) withExemplar(v float64) { } } - e.CounterMetric.Add(v) + c.CounterMetric.Add(v) +} + +func (c *Counter) Add(v float64) { + c.CounterMetric.Add(v) +} + +func (c *Counter) Inc() { + c.CounterMetric.Inc() +} + +func (c *CounterWithContext) Add(v float64) { + c.withExemplar(v) +} + +func (c *CounterWithContext) Inc() { + c.withExemplar(1) } // CounterVec is the internal representation of our wrapping struct around prometheus @@ -298,12 +302,51 @@ type CounterVecWithContext struct { ctx context.Context } +// CounterVecWithContextWithCounter is the wrapper of CounterVecWithContext with counter. +type CounterVecWithContextWithCounter struct { + *CounterVecWithContext + counter CounterMetric +} + +// withExemplar attaches an exemplar to the metric. +func (vcc *CounterVecWithContextWithCounter) withExemplar(v float64) { + if m, ok := vcc.counter.(prometheus.ExemplarAdder); ok { + maybeSpanCtx := trace.SpanContextFromContext(vcc.ctx) + if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { + exemplarLabels := prometheus.Labels{ + "trace_id": maybeSpanCtx.TraceID().String(), + "span_id": maybeSpanCtx.SpanID().String(), + } + m.AddWithExemplar(v, exemplarLabels) + return + } + } + + vcc.counter.Add(v) +} + +// Add adds the given value to the counter with the provided labels. +func (vcc *CounterVecWithContextWithCounter) Add(v float64) { + vcc.withExemplar(v) +} + +// Inc increments the counter with the provided labels. +func (vcc *CounterVecWithContextWithCounter) Inc() { + vcc.withExemplar(1) +} + // WithLabelValues is the wrapper of CounterVec.WithLabelValues. -func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) CounterMetric { - return vc.CounterVec.WithLabelValues(lvs...) +func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) *CounterVecWithContextWithCounter { + return &CounterVecWithContextWithCounter{ + CounterVecWithContext: vc, + counter: vc.CounterVec.WithLabelValues(lvs...), + } } // With is the wrapper of CounterVec.With. -func (vc *CounterVecWithContext) With(labels map[string]string) CounterMetric { - return vc.CounterVec.With(labels) +func (vc *CounterVecWithContext) With(labels map[string]string) *CounterVecWithContextWithCounter { + return &CounterVecWithContextWithCounter{ + CounterVecWithContext: vc, + counter: vc.CounterVec.With(labels), + } } diff --git a/staging/src/k8s.io/component-base/metrics/counter_test.go b/staging/src/k8s.io/component-base/metrics/counter_test.go index 2afcc4d63044c..ea47cab282985 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -292,7 +292,7 @@ func TestCounterWithLabelValueAllowList(t *testing.T) { } func TestCounterWithExemplar(t *testing.T) { - // Set exemplar. + // Create context. fn := func(offset int) []byte { arr := make([]byte, 16) for i := 0; i < 16; i++ { @@ -313,8 +313,7 @@ func TestCounterWithExemplar(t *testing.T) { counter := NewCounter(&CounterOpts{ Name: "metric_exemplar_test", Help: "helpless", - }) - _ = counter.WithContext(ctxForSpanCtx) + }).WithContext(ctxForSpanCtx) // Register counter. registry := newKubeRegistry(apimachineryversion.Info{ @@ -381,4 +380,253 @@ func TestCounterWithExemplar(t *testing.T) { t.Fatalf("Got unexpected label %s", *l.Name) } } + + // Verify that all contextual counter calls are exclusive. + contextualCounter := NewCounter(&CounterOpts{ + Name: "contextual_counter", + Help: "helpless", + }) + spanIDa := trace.SpanID(fn(3)) + traceIDa := trace.TraceID(fn(4)) + contextualCounterA := contextualCounter.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: spanIDa, + TraceID: traceIDa, + TraceFlags: trace.FlagsSampled, + }), + )) + spanIDb := trace.SpanID(fn(5)) + traceIDb := trace.TraceID(fn(6)) + contextualCounterB := contextualCounter.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: spanIDb, + TraceID: traceIDb, + TraceFlags: trace.FlagsSampled, + }), + )) + + runs := []struct { + spanID trace.SpanID + traceID trace.TraceID + contextualCounter *CounterWithContext + }{ + { + spanID: spanIDa, + traceID: traceIDa, + contextualCounter: contextualCounterA, + }, + { + spanID: spanIDb, + traceID: traceIDb, + contextualCounter: contextualCounterB, + }, + } + for _, run := range runs { + registry.MustRegister(run.contextualCounter) + run.contextualCounter.Inc() + + mfs, err = registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != 2 { + t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) + } + + dtoMetric := mfs[0].GetMetric()[0] + if dtoMetric.GetCounter().GetExemplar() == nil { + t.Fatalf("Got nil exemplar, wanted an exemplar") + } + dtoMetricLabels := dtoMetric.GetCounter().GetExemplar().GetLabel() + if len(dtoMetricLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) + } + for _, l := range dtoMetricLabels { + switch *l.Name { + case "trace_id": + if *l.Value != run.traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) + } + case "span_id": + if *l.Value != run.spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) + } + } + + registry.Unregister(run.contextualCounter) + } +} + +func TestCounterVecWithExemplar(t *testing.T) { + // Create context. + fn := func(offset int) []byte { + arr := make([]byte, 16) + for i := 0; i < 16; i++ { + arr[i] = byte(2<<7 - i - offset) + } + return arr + } + traceID := trace.TraceID(fn(1)) + spanID := trace.SpanID(fn(2)) + ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: spanID, + TraceID: traceID, + TraceFlags: trace.FlagsSampled, + }), + ) + toAdd := float64(40) + + // Create contextual counter. + counter := NewCounterVec(&CounterOpts{ + Name: "metricvec_exemplar_test", + Help: "helpless", + }, []string{"label_a"}).WithContext(ctxForSpanCtx) + + // Register counter. + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + registry.MustRegister(counter) + + // Call underlying exemplar methods. + counter.WithLabelValues("a").Add(toAdd) + counter.WithLabelValues("a").Inc() + counter.WithLabelValues("a").Inc() + + // Gather. + mfs, err := registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != 1 { + t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) + } + + // Verify metric type. + mf := mfs[0] + var m *dto.Metric + switch mf.GetType() { + case dto.MetricType_COUNTER: + m = mfs[0].GetMetric()[0] + default: + t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) + } + + // Verify value. + want := toAdd + 2 + got := m.GetCounter().GetValue() + if got != want { + t.Fatalf("Got %f, wanted %f as the count", got, want) + } + + // Verify exemplars. + e := m.GetCounter().GetExemplar() + if e == nil { + t.Fatalf("Got nil exemplar, wanted an exemplar") + } + eLabels := e.GetLabel() + if eLabels == nil { + t.Fatalf("Got nil exemplar label, wanted an exemplar label") + } + if len(eLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) + } + for _, l := range eLabels { + switch *l.Name { + case "trace_id": + if *l.Value != traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String()) + } + case "span_id": + if *l.Value != spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) + } + } + + // Verify that all contextual counter calls are exclusive. + contextualCounterVec := NewCounterVec(&CounterOpts{ + Name: "contextual_counter", + Help: "helpless", + }, []string{"label_a"}) + spanIDa := trace.SpanID(fn(3)) + traceIDa := trace.TraceID(fn(4)) + contextualCounterVecA := contextualCounterVec.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: spanIDa, + TraceID: traceIDa, + TraceFlags: trace.FlagsSampled, + }), + )) + spanIDb := trace.SpanID(fn(5)) + traceIDb := trace.TraceID(fn(6)) + contextualCounterVecB := contextualCounterVec.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: spanIDb, + TraceID: traceIDb, + TraceFlags: trace.FlagsSampled, + }), + )) + + runs := []struct { + spanID trace.SpanID + traceID trace.TraceID + contextualCounter *CounterVecWithContext + }{ + { + spanID: spanIDa, + traceID: traceIDa, + contextualCounter: contextualCounterVecA, + }, + { + spanID: spanIDb, + traceID: traceIDb, + contextualCounter: contextualCounterVecB, + }, + } + for _, run := range runs { + registry.MustRegister(run.contextualCounter) + run.contextualCounter.WithLabelValues("a").Inc() + + mfs, err = registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != 2 { + t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) + } + + dtoMetric := mfs[0].GetMetric()[0] + if dtoMetric.GetCounter().GetExemplar() == nil { + t.Fatalf("Got nil exemplar, wanted an exemplar") + } + dtoMetricLabels := dtoMetric.GetCounter().GetExemplar().GetLabel() + if len(dtoMetricLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) + } + for _, l := range dtoMetricLabels { + switch *l.Name { + case "trace_id": + if *l.Value != run.traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) + } + case "span_id": + if *l.Value != run.spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) + } + } + + registry.Unregister(run.contextualCounter) + } } diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index b410951b67832..d92f2684a7fdc 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -28,34 +28,38 @@ import ( // Histogram is our internal representation for our wrapping struct around prometheus // histograms. Summary implements both kubeCollector and ObserverMetric type Histogram struct { - ctx context.Context ObserverMetric *HistogramOpts lazyMetric selfCollector } -// exemplarHistogramMetric holds a context to extract exemplar labels from, and a historgram metric to attach them to. It implements the metricWithExemplar interface. -type exemplarHistogramMetric struct { +// TODO: make this true: var _ Metric = &Histogram{} + +// HistogramWithContext implements the metricWithExemplar interface. +var _ metricWithExemplar = &HistogramWithContext{} + +// HistogramWithContext holds a context to extract exemplar labels from, and a historgram metric to attach them to. It implements the metricWithExemplar interface. +type HistogramWithContext struct { + ctx context.Context *Histogram } -type exemplarHistogramVec struct { +type HistogramVecWithContextWithObserver struct { *HistogramVecWithContext - observer prometheus.Observer + observer ObserverMetric } func (h *Histogram) Observe(v float64) { - h.withExemplar(v) + h.ObserverMetric.Observe(v) } -// withExemplar initializes the exemplarMetric object and sets the exemplar value. -func (h *Histogram) withExemplar(v float64) { - (&exemplarHistogramMetric{h}).withExemplar(v) +func (e *HistogramWithContext) Observe(v float64) { + e.withExemplar(v) } // withExemplar attaches an exemplar to the metric. -func (e *exemplarHistogramMetric) withExemplar(v float64) { +func (e *HistogramWithContext) withExemplar(v float64) { if m, ok := e.Histogram.ObserverMetric.(prometheus.ExemplarObserver); ok { maybeSpanCtx := trace.SpanContextFromContext(e.ctx) if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { @@ -112,9 +116,11 @@ func (h *Histogram) initializeDeprecatedMetric() { } // WithContext allows the normal Histogram metric to pass in context. The context is no-op now. -func (h *Histogram) WithContext(ctx context.Context) ObserverMetric { - h.ctx = ctx - return h.ObserverMetric +func (h *Histogram) WithContext(ctx context.Context) *HistogramWithContext { + return &HistogramWithContext{ + ctx: ctx, + Histogram: h, + } } // HistogramVec is the internal representation of our wrapping struct around prometheus @@ -268,11 +274,11 @@ type HistogramVecWithContext struct { ctx context.Context } -func (h *exemplarHistogramVec) Observe(v float64) { +func (h *HistogramVecWithContextWithObserver) Observe(v float64) { h.withExemplar(v) } -func (h *exemplarHistogramVec) withExemplar(v float64) { +func (h *HistogramVecWithContextWithObserver) withExemplar(v float64) { if m, ok := h.observer.(prometheus.ExemplarObserver); ok { maybeSpanCtx := trace.SpanContextFromContext(h.HistogramVecWithContext.ctx) if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { @@ -288,16 +294,16 @@ func (h *exemplarHistogramVec) withExemplar(v float64) { } // WithLabelValues is the wrapper of HistogramVec.WithLabelValues. -func (vc *HistogramVecWithContext) WithLabelValues(lvs ...string) *exemplarHistogramVec { - return &exemplarHistogramVec{ +func (vc *HistogramVecWithContext) WithLabelValues(lvs ...string) *HistogramVecWithContextWithObserver { + return &HistogramVecWithContextWithObserver{ HistogramVecWithContext: vc, observer: vc.HistogramVec.WithLabelValues(lvs...), } } // With is the wrapper of HistogramVec.With. -func (vc *HistogramVecWithContext) With(labels map[string]string) *exemplarHistogramVec { - return &exemplarHistogramVec{ +func (vc *HistogramVecWithContext) With(labels map[string]string) *HistogramVecWithContextWithObserver { + return &HistogramVecWithContextWithObserver{ HistogramVecWithContext: vc, observer: vc.HistogramVec.With(labels), } diff --git a/staging/src/k8s.io/component-base/metrics/histogram_test.go b/staging/src/k8s.io/component-base/metrics/histogram_test.go index 5efbfb6eeae2d..3cf4e703912ea 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/histogram_test.go @@ -318,7 +318,7 @@ func TestHistogramWithLabelValueAllowList(t *testing.T) { } func TestHistogramWithExemplar(t *testing.T) { - // Arrange. + // Create context. traceID := trace.TraceID([]byte("trace-0000-xxxxx")) spanID := trace.SpanID([]byte("span-0000-xxxxx")) ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ @@ -328,13 +328,14 @@ func TestHistogramWithExemplar(t *testing.T) { })) value := float64(10) + // Create contextual histogram. histogram := NewHistogram(&HistogramOpts{ Name: "histogram_exemplar_test", Help: "helpless", Buckets: []float64{100}, - }) - _ = histogram.WithContext(ctxForSpanCtx) + }).WithContext(ctxForSpanCtx) + // Register histogram. registry := newKubeRegistry(apimachineryversion.Info{ Major: "1", Minor: "15", @@ -342,53 +343,51 @@ func TestHistogramWithExemplar(t *testing.T) { }) registry.MustRegister(histogram) - // Act. + // Call underlying exemplar methods. histogram.Observe(value) - // Assert. + // Gather. mfs, err := registry.Gather() if err != nil { t.Fatalf("Gather failed %v", err) } - if len(mfs) != 1 { t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) } + // Verify metric type. mf := mfs[0] var m *dto.Metric switch mf.GetType() { case dto.MetricType_HISTOGRAM: m = mfs[0].GetMetric()[0] default: - t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) + t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_HISTOGRAM) } + // Verify value. want := value got := m.GetHistogram().GetSampleSum() if got != want { t.Fatalf("Got %f, wanted %f as the count", got, want) } + // Verify exemplars. buckets := m.GetHistogram().GetBucket() if len(buckets) == 0 { t.Fatalf("Got 0 buckets, wanted 1") } - e := buckets[0].GetExemplar() if e == nil { t.Fatalf("Got nil exemplar, wanted an exemplar") } - eLabels := e.GetLabel() if eLabels == nil { t.Fatalf("Got nil exemplar label, wanted an exemplar label") } - if len(eLabels) != 2 { t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) } - for _, l := range eLabels { switch *l.Name { case "trace_id": @@ -403,10 +402,95 @@ func TestHistogramWithExemplar(t *testing.T) { t.Fatalf("Got unexpected label %s", *l.Name) } } + + // Verify that all contextual histogram calls are exclusive. + contextualHistogram := NewHistogram(&HistogramOpts{ + Name: "contextual_histogram", + Help: "helpless", + Buckets: []float64{100}, + }) + traceIDa := trace.TraceID([]byte("trace-0000-aaaaa")) + spanIDa := trace.SpanID([]byte("span-0000-aaaaa")) + contextualHistogramA := contextualHistogram.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceIDa, + SpanID: spanIDa, + TraceFlags: trace.FlagsSampled, + }), + )) + traceIDb := trace.TraceID([]byte("trace-0000-bbbbb")) + spanIDb := trace.SpanID([]byte("span-0000-bbbbb")) + contextualHistogramB := contextualHistogram.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceIDb, + SpanID: spanIDb, + TraceFlags: trace.FlagsSampled, + }), + )) + + runs := []struct { + spanID trace.SpanID + traceID trace.TraceID + contextualHistogram *HistogramWithContext + }{ + { + spanID: spanIDa, + traceID: traceIDa, + contextualHistogram: contextualHistogramA, + }, + { + spanID: spanIDb, + traceID: traceIDb, + contextualHistogram: contextualHistogramB, + }, + } + for _, run := range runs { + registry.MustRegister(run.contextualHistogram) + run.contextualHistogram.Observe(value) + + mfs, err = registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != 2 { + t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) + } + + dtoMetric := mfs[0].GetMetric()[0] + dtoMetricBuckets := dtoMetric.GetHistogram().GetBucket() + if len(dtoMetricBuckets) == 0 { + t.Fatalf("Got nil buckets") + } + dtoMetricBucketsExemplar := dtoMetricBuckets[0].GetExemplar() + if dtoMetricBucketsExemplar == nil { + t.Fatalf("Got nil exemplar") + } + + dtoMetricLabels := dtoMetricBucketsExemplar.GetLabel() + if len(dtoMetricLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) + } + for _, l := range dtoMetricLabels { + switch *l.Name { + case "trace_id": + if *l.Value != run.traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) + } + case "span_id": + if *l.Value != run.spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) + } + } + + registry.Unregister(run.contextualHistogram) + } } func TestHistogramVecWithExemplar(t *testing.T) { - // Arrange. + // Create context. traceID := trace.TraceID([]byte("trace-0000-xxxxx")) spanID := trace.SpanID([]byte("span-0000-xxxxx")) ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ @@ -416,6 +500,7 @@ func TestHistogramVecWithExemplar(t *testing.T) { })) value := float64(10) + // Create contextual histogram. histogramVec := NewHistogramVec(&HistogramOpts{ Name: "histogram_exemplar_test", Help: "helpless", @@ -423,6 +508,7 @@ func TestHistogramVecWithExemplar(t *testing.T) { }, []string{"group"}) h := histogramVec.WithContext(ctxForSpanCtx) + // Register histogram. registry := newKubeRegistry(apimachineryversion.Info{ Major: "1", Minor: "15", @@ -430,53 +516,51 @@ func TestHistogramVecWithExemplar(t *testing.T) { }) registry.MustRegister(histogramVec) - // Act. + // Call underlying exemplar methods. h.WithLabelValues("foo").Observe(value) - // Assert. + // Gather. mfs, err := registry.Gather() if err != nil { t.Fatalf("Gather failed %v", err) } - if len(mfs) != 1 { t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) } + // Verify metric type. mf := mfs[0] var m *dto.Metric switch mf.GetType() { case dto.MetricType_HISTOGRAM: m = mfs[0].GetMetric()[0] default: - t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) + t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_HISTOGRAM) } + // Verify value. want := value got := m.GetHistogram().GetSampleSum() if got != want { t.Fatalf("Got %f, wanted %f as the count", got, want) } + // Verify exemplars. buckets := m.GetHistogram().GetBucket() if len(buckets) == 0 { t.Fatalf("Got 0 buckets, wanted 1") } - e := buckets[0].GetExemplar() if e == nil { t.Fatalf("Got nil exemplar, wanted an exemplar") } - eLabels := e.GetLabel() if eLabels == nil { t.Fatalf("Got nil exemplar label, wanted an exemplar label") } - if len(eLabels) != 2 { t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) } - for _, l := range eLabels { switch *l.Name { case "trace_id": @@ -491,4 +575,87 @@ func TestHistogramVecWithExemplar(t *testing.T) { t.Fatalf("Got unexpected label %s", *l.Name) } } + + // Verify that all contextual histogram calls are exclusive. + contextualHistogramVec := NewHistogramVec(&HistogramOpts{ + Name: "contextual_histogram_vec", + Help: "helpless", + Buckets: []float64{100}, + }, []string{"group"}) + traceIDa := trace.TraceID([]byte("trace-0000-aaaaa")) + spanIDa := trace.SpanID([]byte("span-0000-aaaaa")) + contextualHistogramVecA := contextualHistogramVec.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceIDa, + SpanID: spanIDa, + TraceFlags: trace.FlagsSampled, + }), + )) + traceIDb := trace.TraceID([]byte("trace-0000-bbbbb")) + spanIDb := trace.SpanID([]byte("span-0000-bbbbb")) + contextualHistogramVecB := contextualHistogramVec.WithContext(trace.ContextWithSpanContext(context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceIDb, + SpanID: spanIDb, + TraceFlags: trace.FlagsSampled, + }), + )) + runs := []struct { + spanID trace.SpanID + traceID trace.TraceID + contextualHistogramVec *HistogramVecWithContext + }{ + { + spanID: spanIDa, + traceID: traceIDa, + contextualHistogramVec: contextualHistogramVecA, + }, { + spanID: spanIDb, + traceID: traceIDb, + contextualHistogramVec: contextualHistogramVecB, + }, + } + for _, run := range runs { + registry.MustRegister(run.contextualHistogramVec) + run.contextualHistogramVec.WithLabelValues("foo").Observe(value) + + mfs, err = registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != 2 { + t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) + } + + dtoMetric := mfs[0].GetMetric()[0] + dtoMetricBuckets := dtoMetric.GetHistogram().GetBucket() + if len(dtoMetricBuckets) == 0 { + t.Fatalf("Got nil buckets") + } + dtoMetricBucketsExemplar := dtoMetricBuckets[0].GetExemplar() + if dtoMetricBucketsExemplar == nil { + t.Fatalf("Got nil exemplar") + } + + dtoMetricLabels := dtoMetricBucketsExemplar.GetLabel() + if len(dtoMetricLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) + } + for _, l := range dtoMetricLabels { + switch *l.Name { + case "trace_id": + if *l.Value != run.traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) + } + case "span_id": + if *l.Value != run.spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) + } + } + + registry.Unregister(run.contextualHistogramVec) + } } From 5d048f12bb7cea1361feda065186cce36d1f21e4 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 19 Dec 2024 13:01:05 +0530 Subject: [PATCH 2/5] fixup! address `metrics.(*Counter)` context data-race --- staging/src/k8s.io/component-base/metrics/counter.go | 2 +- staging/src/k8s.io/component-base/metrics/histogram.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 31f113603e191..d3c6f47b1bba9 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -37,8 +37,8 @@ type Counter struct { } type CounterWithContext struct { - ctx context.Context *Counter + ctx context.Context } // The implementation of the Metric interface is expected by testutil.GetCounterMetricValue. diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index d92f2684a7fdc..a20e3b8412fae 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -41,8 +41,8 @@ var _ metricWithExemplar = &HistogramWithContext{} // HistogramWithContext holds a context to extract exemplar labels from, and a historgram metric to attach them to. It implements the metricWithExemplar interface. type HistogramWithContext struct { - ctx context.Context *Histogram + ctx context.Context } type HistogramVecWithContextWithObserver struct { From 402728676cdc8326cdc803bd1e1ff00de814ecf5 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 6 Mar 2025 03:38:04 +0530 Subject: [PATCH 3/5] fixup! fixup! address `metrics.(*Counter)` context data-race --- .../k8s.io/component-base/metrics/counter.go | 87 +++------ .../component-base/metrics/counter_test.go | 171 ------------------ 2 files changed, 29 insertions(+), 229 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index d3c6f47b1bba9..8aaa36e76034e 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -36,17 +36,12 @@ type Counter struct { selfCollector } -type CounterWithContext struct { - *Counter - ctx context.Context -} +// The metric must be register-able. +var _ Registerable = &Counter{} // The implementation of the Metric interface is expected by testutil.GetCounterMetricValue. var _ Metric = &Counter{} -// All supported exemplar metric types implement the metricWithExemplar interface. -var _ metricWithExemplar = &CounterWithContext{} - // NewCounter returns an object which satisfies the kubeCollector and CounterMetric interfaces. // However, the object returned will not measure anything unless the collector is first // registered, since the metric is lazily instantiated. @@ -104,8 +99,31 @@ func (c *Counter) initializeDeprecatedMetric() { c.initializeMetric() } +func (c *Counter) Add(v float64) { + c.CounterMetric.Add(v) +} + +func (c *Counter) Inc() { + c.CounterMetric.Inc() +} + +// CounterWithContext is the wrapper of Counter with context. +// It allows for passing contextual counters without copying their internal lock(s). +type CounterWithContext struct { + ctx context.Context + *Counter +} + +// Post-equipping a counter with context, folks should be able to use it as a regular counter, with exemplar support. +var _ Registerable = &Counter{} +var _ Metric = &CounterWithContext{} +var _ metricWithExemplar = &CounterWithContext{} + // WithContext allows the normal Counter metric to pass in context. func (c *Counter) WithContext(ctx context.Context) *CounterWithContext { + // Return reference to a new counter as modifying the existing one overrides the older context, + // and blocks with semaphores. So this is a better option, see: + // https://github.com/kubernetes/kubernetes/pull/128575#discussion_r1829509144. return &CounterWithContext{ ctx: ctx, Counter: c, @@ -129,14 +147,6 @@ func (c *CounterWithContext) withExemplar(v float64) { c.CounterMetric.Add(v) } -func (c *Counter) Add(v float64) { - c.CounterMetric.Add(v) -} - -func (c *Counter) Inc() { - c.CounterMetric.Inc() -} - func (c *CounterWithContext) Add(v float64) { c.withExemplar(v) } @@ -302,51 +312,12 @@ type CounterVecWithContext struct { ctx context.Context } -// CounterVecWithContextWithCounter is the wrapper of CounterVecWithContext with counter. -type CounterVecWithContextWithCounter struct { - *CounterVecWithContext - counter CounterMetric -} - -// withExemplar attaches an exemplar to the metric. -func (vcc *CounterVecWithContextWithCounter) withExemplar(v float64) { - if m, ok := vcc.counter.(prometheus.ExemplarAdder); ok { - maybeSpanCtx := trace.SpanContextFromContext(vcc.ctx) - if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { - exemplarLabels := prometheus.Labels{ - "trace_id": maybeSpanCtx.TraceID().String(), - "span_id": maybeSpanCtx.SpanID().String(), - } - m.AddWithExemplar(v, exemplarLabels) - return - } - } - - vcc.counter.Add(v) -} - -// Add adds the given value to the counter with the provided labels. -func (vcc *CounterVecWithContextWithCounter) Add(v float64) { - vcc.withExemplar(v) -} - -// Inc increments the counter with the provided labels. -func (vcc *CounterVecWithContextWithCounter) Inc() { - vcc.withExemplar(1) -} - // WithLabelValues is the wrapper of CounterVec.WithLabelValues. -func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) *CounterVecWithContextWithCounter { - return &CounterVecWithContextWithCounter{ - CounterVecWithContext: vc, - counter: vc.CounterVec.WithLabelValues(lvs...), - } +func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) CounterMetric { + return vc.CounterVec.WithLabelValues(lvs...) } // With is the wrapper of CounterVec.With. -func (vc *CounterVecWithContext) With(labels map[string]string) *CounterVecWithContextWithCounter { - return &CounterVecWithContextWithCounter{ - CounterVecWithContext: vc, - counter: vc.CounterVec.With(labels), - } +func (vc *CounterVecWithContext) With(labels map[string]string) CounterMetric { + return vc.CounterVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/counter_test.go b/staging/src/k8s.io/component-base/metrics/counter_test.go index ea47cab282985..2dbeeb09c7c2e 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -459,174 +459,3 @@ func TestCounterWithExemplar(t *testing.T) { registry.Unregister(run.contextualCounter) } } - -func TestCounterVecWithExemplar(t *testing.T) { - // Create context. - fn := func(offset int) []byte { - arr := make([]byte, 16) - for i := 0; i < 16; i++ { - arr[i] = byte(2<<7 - i - offset) - } - return arr - } - traceID := trace.TraceID(fn(1)) - spanID := trace.SpanID(fn(2)) - ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), - trace.NewSpanContext(trace.SpanContextConfig{ - SpanID: spanID, - TraceID: traceID, - TraceFlags: trace.FlagsSampled, - }), - ) - toAdd := float64(40) - - // Create contextual counter. - counter := NewCounterVec(&CounterOpts{ - Name: "metricvec_exemplar_test", - Help: "helpless", - }, []string{"label_a"}).WithContext(ctxForSpanCtx) - - // Register counter. - registry := newKubeRegistry(apimachineryversion.Info{ - Major: "1", - Minor: "15", - GitVersion: "v1.15.0-alpha-1.12345", - }) - registry.MustRegister(counter) - - // Call underlying exemplar methods. - counter.WithLabelValues("a").Add(toAdd) - counter.WithLabelValues("a").Inc() - counter.WithLabelValues("a").Inc() - - // Gather. - mfs, err := registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 1 { - t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) - } - - // Verify metric type. - mf := mfs[0] - var m *dto.Metric - switch mf.GetType() { - case dto.MetricType_COUNTER: - m = mfs[0].GetMetric()[0] - default: - t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) - } - - // Verify value. - want := toAdd + 2 - got := m.GetCounter().GetValue() - if got != want { - t.Fatalf("Got %f, wanted %f as the count", got, want) - } - - // Verify exemplars. - e := m.GetCounter().GetExemplar() - if e == nil { - t.Fatalf("Got nil exemplar, wanted an exemplar") - } - eLabels := e.GetLabel() - if eLabels == nil { - t.Fatalf("Got nil exemplar label, wanted an exemplar label") - } - if len(eLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) - } - for _, l := range eLabels { - switch *l.Name { - case "trace_id": - if *l.Value != traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String()) - } - case "span_id": - if *l.Value != spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) - } - } - - // Verify that all contextual counter calls are exclusive. - contextualCounterVec := NewCounterVec(&CounterOpts{ - Name: "contextual_counter", - Help: "helpless", - }, []string{"label_a"}) - spanIDa := trace.SpanID(fn(3)) - traceIDa := trace.TraceID(fn(4)) - contextualCounterVecA := contextualCounterVec.WithContext(trace.ContextWithSpanContext(context.Background(), - trace.NewSpanContext(trace.SpanContextConfig{ - SpanID: spanIDa, - TraceID: traceIDa, - TraceFlags: trace.FlagsSampled, - }), - )) - spanIDb := trace.SpanID(fn(5)) - traceIDb := trace.TraceID(fn(6)) - contextualCounterVecB := contextualCounterVec.WithContext(trace.ContextWithSpanContext(context.Background(), - trace.NewSpanContext(trace.SpanContextConfig{ - SpanID: spanIDb, - TraceID: traceIDb, - TraceFlags: trace.FlagsSampled, - }), - )) - - runs := []struct { - spanID trace.SpanID - traceID trace.TraceID - contextualCounter *CounterVecWithContext - }{ - { - spanID: spanIDa, - traceID: traceIDa, - contextualCounter: contextualCounterVecA, - }, - { - spanID: spanIDb, - traceID: traceIDb, - contextualCounter: contextualCounterVecB, - }, - } - for _, run := range runs { - registry.MustRegister(run.contextualCounter) - run.contextualCounter.WithLabelValues("a").Inc() - - mfs, err = registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 2 { - t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) - } - - dtoMetric := mfs[0].GetMetric()[0] - if dtoMetric.GetCounter().GetExemplar() == nil { - t.Fatalf("Got nil exemplar, wanted an exemplar") - } - dtoMetricLabels := dtoMetric.GetCounter().GetExemplar().GetLabel() - if len(dtoMetricLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) - } - for _, l := range dtoMetricLabels { - switch *l.Name { - case "trace_id": - if *l.Value != run.traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) - } - case "span_id": - if *l.Value != run.spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) - } - } - - registry.Unregister(run.contextualCounter) - } -} From 61f7358ef77b49f06e612df3653907fe32eae28c Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 9 Mar 2025 19:51:36 +0530 Subject: [PATCH 4/5] fixup! fixup! fixup! address `metrics.(*Counter)` context data-race --- .../k8s.io/component-base/metrics/counter.go | 25 +-- .../component-base/metrics/counter_test.go | 171 +++++++++--------- 2 files changed, 91 insertions(+), 105 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 8aaa36e76034e..840b2ea877164 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -298,26 +298,7 @@ func (v *CounterVec) ResetLabelAllowLists() { v.LabelValueAllowLists = nil } -// WithContext returns wrapped CounterVec with context -func (v *CounterVec) WithContext(ctx context.Context) *CounterVecWithContext { - return &CounterVecWithContext{ - ctx: ctx, - CounterVec: v, - } -} - -// CounterVecWithContext is the wrapper of CounterVec with context. -type CounterVecWithContext struct { - *CounterVec - ctx context.Context -} - -// WithLabelValues is the wrapper of CounterVec.WithLabelValues. -func (vc *CounterVecWithContext) WithLabelValues(lvs ...string) CounterMetric { - return vc.CounterVec.WithLabelValues(lvs...) -} - -// With is the wrapper of CounterVec.With. -func (vc *CounterVecWithContext) With(labels map[string]string) CounterMetric { - return vc.CounterVec.With(labels) +// WithContext is a no-op. +func (v *CounterVec) WithContext(_ context.Context) *CounterVec { + return v } diff --git a/staging/src/k8s.io/component-base/metrics/counter_test.go b/staging/src/k8s.io/component-base/metrics/counter_test.go index 2dbeeb09c7c2e..8448f254626b4 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -300,6 +300,13 @@ func TestCounterWithExemplar(t *testing.T) { } return arr } + originalTraceID := trace.TraceID(fn(0)) + originalSpanID := trace.SpanID(fn(1)) + redundantCtxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: originalSpanID, + TraceID: originalTraceID, + TraceFlags: trace.FlagsSampled, + })) traceID := trace.TraceID(fn(1)) spanID := trace.SpanID(fn(2)) ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ @@ -323,63 +330,30 @@ func TestCounterWithExemplar(t *testing.T) { }) registry.MustRegister(counter) - // Call underlying exemplar methods. - counter.Add(toAdd) - counter.Inc() - counter.Inc() + // Call underlying exemplar methods, overriding the original span context. + counter.WithContext(redundantCtxForSpanCtx).Add(toAdd) // This should be counted, but it's context should be overridden. + counter.WithContext(redundantCtxForSpanCtx).Inc() // This should be counted, but it's context should be overridden. + counter.Inc() // This should be counted, and it's (parent's) context should be used. - // Gather. - mfs, err := registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 1 { - t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) - } + // Verify exemplar with the overridden span context. + exemplarGatherAndVerify(t, registry, traceID, spanID, 1, toAdd+2) - // Verify metric type. - mf := mfs[0] - var m *dto.Metric - switch mf.GetType() { - case dto.MetricType_COUNTER: - m = mfs[0].GetMetric()[0] - default: - t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) - } + // Modify span context. + altTraceID := trace.TraceID(fn(2)) + altSpanID := trace.SpanID(fn(3)) + altCtxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ + SpanID: altSpanID, + TraceID: altTraceID, + TraceFlags: trace.FlagsSampled, + })) - // Verify value. - want := toAdd + 2 - got := m.GetCounter().GetValue() - if got != want { - t.Fatalf("Got %f, wanted %f as the count", got, want) - } + // Call underlying exemplar methods with different span context. + counter.WithContext(ctxForSpanCtx).Add(toAdd) // This should be counted, but it's context should be overridden. + counter.WithContext(ctxForSpanCtx).Inc() // This should be counted, but it's context should be overridden. + counter.WithContext(altCtxForSpanCtx).Inc() // This should be counted, and it's context should be used. - // Verify exemplars. - e := m.GetCounter().GetExemplar() - if e == nil { - t.Fatalf("Got nil exemplar, wanted an exemplar") - } - eLabels := e.GetLabel() - if eLabels == nil { - t.Fatalf("Got nil exemplar label, wanted an exemplar label") - } - if len(eLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) - } - for _, l := range eLabels { - switch *l.Name { - case "trace_id": - if *l.Value != traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String()) - } - case "span_id": - if *l.Value != spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) - } - } + // Verify exemplar with different span context. + exemplarGatherAndVerify(t, registry, altTraceID, altSpanID, 1, 2*toAdd+4) // Verify that all contextual counter calls are exclusive. contextualCounter := NewCounter(&CounterOpts{ @@ -421,41 +395,72 @@ func TestCounterWithExemplar(t *testing.T) { contextualCounter: contextualCounterB, }, } - for _, run := range runs { + for i, run := range runs { registry.MustRegister(run.contextualCounter) run.contextualCounter.Inc() + exemplarGatherAndVerify(t, registry, run.traceID, run.spanID, 2, float64(i+1)) + registry.Unregister(run.contextualCounter) + } +} - mfs, err = registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 2 { - t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) - } +func exemplarGatherAndVerify(t *testing.T, + registry *kubeRegistry, + traceID trace.TraceID, + spanID trace.SpanID, + expectedMetricFamilies int, + expectedValue float64, +) { - dtoMetric := mfs[0].GetMetric()[0] - if dtoMetric.GetCounter().GetExemplar() == nil { - t.Fatalf("Got nil exemplar, wanted an exemplar") - } - dtoMetricLabels := dtoMetric.GetCounter().GetExemplar().GetLabel() - if len(dtoMetricLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) - } - for _, l := range dtoMetricLabels { - switch *l.Name { - case "trace_id": - if *l.Value != run.traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) - } - case "span_id": - if *l.Value != run.spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) + // Gather. + mfs, err := registry.Gather() + if err != nil { + t.Fatalf("Gather failed %v", err) + } + if len(mfs) != expectedMetricFamilies { + t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) + } + + // Verify metric type. + mf := mfs[0] + var m *dto.Metric + switch mf.GetType() { + case dto.MetricType_COUNTER: + m = mfs[0].GetMetric()[0] + default: + t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) + } + + // Verify value. + want := expectedValue + got := m.GetCounter().GetValue() + if got != want { + t.Fatalf("Got %f, wanted %f as the count", got, want) + } + + // Verify exemplars. + e := m.GetCounter().GetExemplar() + if e == nil { + t.Fatalf("Got nil exemplar, wanted an exemplar") + } + eLabels := e.GetLabel() + if eLabels == nil { + t.Fatalf("Got nil exemplar label, wanted an exemplar label") + } + if len(eLabels) != 2 { + t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) + } + for _, l := range eLabels { + switch *l.Name { + case "trace_id": + if *l.Value != traceID.String() { + t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String()) } + case "span_id": + if *l.Value != spanID.String() { + t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String()) + } + default: + t.Fatalf("Got unexpected label %s", *l.Name) } - - registry.Unregister(run.contextualCounter) } } From 1b5b50bd26a35b7df3be5d966d39c8fb2eeb6836 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 24 Jul 2025 13:38:30 +0530 Subject: [PATCH 5/5] fixup! fixup! fixup! fixup! address `metrics.(*Counter)` context data-race --- .../k8s.io/component-base/metrics/counter.go | 65 +++--- .../component-base/metrics/counter_test.go | 66 +++++++ .../component-base/metrics/histogram.go | 187 ++++++++---------- .../component-base/metrics/histogram_test.go | 178 ++++------------- 4 files changed, 218 insertions(+), 278 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 840b2ea877164..6da1108afb516 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -42,6 +42,9 @@ var _ Registerable = &Counter{} // The implementation of the Metric interface is expected by testutil.GetCounterMetricValue. var _ Metric = &Counter{} +// The implementation of kubeCollector is expected for collector registration. +var _ kubeCollector = &Counter{} + // NewCounter returns an object which satisfies the kubeCollector and CounterMetric interfaces. // However, the object returned will not measure anything unless the collector is first // registered, since the metric is lazily instantiated. @@ -57,6 +60,27 @@ func NewCounter(opts *CounterOpts) *Counter { return kc } +// initializeDeprecatedMetric invocation creates the actual (but deprecated) Counter. Until this method +// is called the underlying counter is a no-op. +func (c *Counter) initializeDeprecatedMetric() { + c.CounterOpts.markDeprecated() + c.initializeMetric() +} + +// initializeMetric invocation creates the actual underlying Counter. Until this method is called +// the underlying counter is a no-op. +func (c *Counter) initializeMetric() { + c.CounterOpts.annotateStabilityLevel() + // this actually creates the underlying prometheus counter. + c.setPrometheusCounter(prometheus.NewCounter(c.CounterOpts.toPromCounterOpts())) +} + +// setPrometheusCounter sets the underlying CounterMetric object, i.e. the thing that does the measurement. +func (c *Counter) setPrometheusCounter(counter prometheus.Counter) { + c.CounterMetric = counter + c.initSelfCollection(counter) +} + func (c *Counter) Desc() *prometheus.Desc { return c.metric.Desc() } @@ -73,32 +97,11 @@ func (c *Counter) Reset() { c.setPrometheusCounter(prometheus.NewCounter(c.CounterOpts.toPromCounterOpts())) } -// setPrometheusCounter sets the underlying CounterMetric object, i.e. the thing that does the measurement. -func (c *Counter) setPrometheusCounter(counter prometheus.Counter) { - c.CounterMetric = counter - c.initSelfCollection(counter) -} - // DeprecatedVersion returns a pointer to the Version or nil func (c *Counter) DeprecatedVersion() *semver.Version { return parseSemver(c.CounterOpts.DeprecatedVersion) } -// initializeMetric invocation creates the actual underlying Counter. Until this method is called -// the underlying counter is a no-op. -func (c *Counter) initializeMetric() { - c.CounterOpts.annotateStabilityLevel() - // this actually creates the underlying prometheus counter. - c.setPrometheusCounter(prometheus.NewCounter(c.CounterOpts.toPromCounterOpts())) -} - -// initializeDeprecatedMetric invocation creates the actual (but deprecated) Counter. Until this method -// is called the underlying counter is a no-op. -func (c *Counter) initializeDeprecatedMetric() { - c.CounterOpts.markDeprecated() - c.initializeMetric() -} - func (c *Counter) Add(v float64) { c.CounterMetric.Add(v) } @@ -115,11 +118,10 @@ type CounterWithContext struct { } // Post-equipping a counter with context, folks should be able to use it as a regular counter, with exemplar support. -var _ Registerable = &Counter{} var _ Metric = &CounterWithContext{} var _ metricWithExemplar = &CounterWithContext{} -// WithContext allows the normal Counter metric to pass in context. +// WithContext allows a Counter to bind to a context. func (c *Counter) WithContext(ctx context.Context) *CounterWithContext { // Return reference to a new counter as modifying the existing one overrides the older context, // and blocks with semaphores. So this is a better option, see: @@ -164,14 +166,13 @@ type CounterVec struct { originalLabels []string } +// The implementation of kubeCollector is expected for collector registration. +// CounterVec implements the kubeCollector, but not the CounterVecMetric interface. var _ kubeCollector = &CounterVec{} -// TODO: make this true: var _ CounterVecMetric = &CounterVec{} - -// NewCounterVec returns an object which satisfies the kubeCollector and (almost) CounterVecMetric interfaces. -// However, the object returned will not measure anything unless the collector is first -// registered, since the metric is lazily instantiated, and only members extracted after -// registration will actually measure anything. +// NewCounterVec returns an object which satisfies the kubeCollector and (almost) CounterVecMetric interfaces. However, +// the object returned will not measure anything unless the collector is first registered, since the metric is lazily +// instantiated, and only members extracted after registration will actually measure anything. func NewCounterVec(opts *CounterOpts, labels []string) *CounterVec { opts.StabilityLevel.setDefaults() @@ -248,6 +249,7 @@ func (v *CounterVec) WithLabelValues(lvs ...string) CounterMetric { // must match those of the VariableLabels in Desc). If that label map is // accessed for the first time, a new Counter is created IFF the counterVec has // been registered to a metrics registry. +// TODO: Do a k8s-wide refactor changing this to prometheus.Labels so CounterVec actually implements CounterVecMetric func (v *CounterVec) With(labels map[string]string) CounterMetric { if !v.IsCreated() { return noop // return no-op counter @@ -298,7 +300,10 @@ func (v *CounterVec) ResetLabelAllowLists() { v.LabelValueAllowLists = nil } -// WithContext is a no-op. +// WithContext is a no-op for now, users should still attach this to vectors for seamless future API upgrades (which rely on the context). +// This is done to keep extensions (such as exemplars) on the counter and not its derivatives. +// Note that there are no actual uses for this in the codebase except for chaining it with `WithLabelValues`, which makes no use of the context. +// Furthermore, Prometheus, which is upstream from this library, does not support contextual vectors, so we don't want to diverge. func (v *CounterVec) WithContext(_ context.Context) *CounterVec { return v } diff --git a/staging/src/k8s.io/component-base/metrics/counter_test.go b/staging/src/k8s.io/component-base/metrics/counter_test.go index 8448f254626b4..1068ef82ce354 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/blang/semver/v4" + "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "github.com/prometheus/common/expfmt" "github.com/stretchr/testify/assert" @@ -464,3 +465,68 @@ func exemplarGatherAndVerify(t *testing.T, } } } + +// Prometheus does not support exemplars for metric vectors, but only the metric scalars themselves. +// The following is the only way Prometheus supports exemplars for vectors. +// We test this to make sure the same expectations are met. +func TestCounterVecWithExemplar(t *testing.T) { + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + counterVec := NewCounterVec(&CounterOpts{ + Name: "metric_test_name", + Help: "counter help", + Subsystem: "subsystem", + Namespace: "namespace", + }, []string{"label_a", "label_b"}) + registry.MustRegister(counterVec) + + // no-op, but this shouldn't panic. + c := counterVec.WithContext(nil) + + c.WithLabelValues("1", "2").Inc() + c.WithLabelValues("1", "2").(prometheus.ExemplarAdder).AddWithExemplar(100, prometheus.Labels{ + "exemplar_label": "42", + }) + + mfs, err := registry.Gather() + require.NoError(t, err, "Gather failed %v", err) + + mf := mfs[0] + + assert.Lenf(t, mfs, 1, "Got %v metric families, Want: 1 metric family", len(mfs)) + assert.Equal(t, BuildFQName("namespace", "subsystem", "metric_test_name"), *mf.Name) + assert.Equal(t, "[ALPHA] counter help", mf.GetHelp()) + assert.Lenf(t, mf.GetMetric(), 1, "Got %v metrics, wanted 1 as the count", len(mf.GetMetric())) + + var m *dto.Metric + switch mf.GetType() { + case dto.MetricType_COUNTER: + m = mfs[0].GetMetric()[0] + default: + t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_COUNTER) + } + + var aValue, bValue string + for _, l := range m.GetLabel() { + if *l.Name == "label_a" { + aValue = *l.Value + } + if *l.Name == "label_b" { + bValue = *l.Value + } + } + labelValuePair := aValue + " " + bValue + gotValue := int(m.GetCounter().GetValue()) + gotExemplarValue := int(m.GetCounter().GetExemplar().GetValue()) + gotExemplarLabelName := m.GetCounter().GetExemplar().GetLabel()[0].GetName() + gotExemplarLabelValue := m.GetCounter().GetExemplar().GetLabel()[0].GetValue() + + assert.Equalf(t, "1 2", labelValuePair, "Got %v, wanted %v as the label values", labelValuePair, "1 2") + assert.Equalf(t, 101, gotValue, "Got %v, wanted %v as the count while setting label_a to %v and label b to %v", gotValue, 101, aValue, bValue) + assert.Equalf(t, 100, gotExemplarValue, "Got %v, wanted %v as the exemplar value while setting label_a to %v and label b to %v", gotExemplarValue, 100, aValue, bValue) + assert.Equalf(t, "exemplar_label", gotExemplarLabelName, "Got %v, wanted %v as the exemplar label name", gotExemplarLabelName, "exemplar_label") + assert.Equalf(t, "42", gotExemplarLabelValue, "Got %v, wanted %v as the exemplar label value", gotExemplarLabelValue, "42") +} diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index a20e3b8412fae..30e67e61ea016 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -22,11 +22,11 @@ import ( "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" "go.opentelemetry.io/otel/trace" ) -// Histogram is our internal representation for our wrapping struct around prometheus -// histograms. Summary implements both kubeCollector and ObserverMetric +// Histogram is our internal representation for our wrapping struct around prometheus histograms. type Histogram struct { ObserverMetric *HistogramOpts @@ -34,46 +34,14 @@ type Histogram struct { selfCollector } -// TODO: make this true: var _ Metric = &Histogram{} +// The metric must be register-able. +var _ Registerable = &Histogram{} -// HistogramWithContext implements the metricWithExemplar interface. -var _ metricWithExemplar = &HistogramWithContext{} - -// HistogramWithContext holds a context to extract exemplar labels from, and a historgram metric to attach them to. It implements the metricWithExemplar interface. -type HistogramWithContext struct { - *Histogram - ctx context.Context -} +// The implementation of the Metric interface is expected by testutil.GetHistogramMetricValue. +var _ Metric = &Histogram{} -type HistogramVecWithContextWithObserver struct { - *HistogramVecWithContext - observer ObserverMetric -} - -func (h *Histogram) Observe(v float64) { - h.ObserverMetric.Observe(v) -} - -func (e *HistogramWithContext) Observe(v float64) { - e.withExemplar(v) -} - -// withExemplar attaches an exemplar to the metric. -func (e *HistogramWithContext) withExemplar(v float64) { - if m, ok := e.Histogram.ObserverMetric.(prometheus.ExemplarObserver); ok { - maybeSpanCtx := trace.SpanContextFromContext(e.ctx) - if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { - exemplarLabels := prometheus.Labels{ - "trace_id": maybeSpanCtx.TraceID().String(), - "span_id": maybeSpanCtx.SpanID().String(), - } - m.ObserveWithExemplar(v, exemplarLabels) - return - } - } - - e.ObserverMetric.Observe(v) -} +// The implementation of kubeCollector is expected for collector registration. +var _ kubeCollector = &Histogram{} // NewHistogram returns an object which is Histogram-like. However, nothing // will be measured until the histogram is registered somewhere. @@ -89,40 +57,91 @@ func NewHistogram(opts *HistogramOpts) *Histogram { return h } +// initializeDeprecatedMetric invokes the actual prometheus.Histogram object instantiation +// but modifies the Help description prior to object instantiation. +func (h *Histogram) initializeDeprecatedMetric() { + h.HistogramOpts.markDeprecated() + h.initializeMetric() +} + +// initializeMetric invokes the actual prometheus.Histogram object instantiation +// and stores a reference to it +func (h *Histogram) initializeMetric() { + h.HistogramOpts.annotateStabilityLevel() + // this actually creates the underlying prometheus gauge. + h.setPrometheusHistogram(prometheus.NewHistogram(h.HistogramOpts.toPromHistogramOpts())) +} + // setPrometheusHistogram sets the underlying KubeGauge object, i.e. the thing that does the measurement. func (h *Histogram) setPrometheusHistogram(histogram prometheus.Histogram) { h.ObserverMetric = histogram h.initSelfCollection(histogram) } +// Desc returns the prometheus.Desc for the metric. +// This method is required by the Metric interface. +func (h *Histogram) Desc() *prometheus.Desc { + return h.metric.Desc() +} + +// Write writes the metric to the provided dto.Metric. +// This method is required by the Metric interface. +func (h *Histogram) Write(to *dto.Metric) error { + return h.metric.Write(to) +} + // DeprecatedVersion returns a pointer to the Version or nil func (h *Histogram) DeprecatedVersion() *semver.Version { return parseSemver(h.HistogramOpts.DeprecatedVersion) } -// initializeMetric invokes the actual prometheus.Histogram object instantiation -// and stores a reference to it -func (h *Histogram) initializeMetric() { - h.HistogramOpts.annotateStabilityLevel() - // this actually creates the underlying prometheus gauge. - h.setPrometheusHistogram(prometheus.NewHistogram(h.HistogramOpts.toPromHistogramOpts())) +// Observe is the method that implements the ObserverMetric interface. +func (h *Histogram) Observe(v float64) { + h.ObserverMetric.Observe(v) } -// initializeDeprecatedMetric invokes the actual prometheus.Histogram object instantiation -// but modifies the Help description prior to object instantiation. -func (h *Histogram) initializeDeprecatedMetric() { - h.HistogramOpts.markDeprecated() - h.initializeMetric() +// HistogramWithContext holds a context to extract exemplar labels from, and a historgram metric to attach them to. It implements the metricWithExemplar interface. +type HistogramWithContext struct { + ctx context.Context + *Histogram } -// WithContext allows the normal Histogram metric to pass in context. The context is no-op now. +// Post-equipping a histogram with context, folks should be able to use it as a regular histogram, with exemplar support. +var _ Metric = &HistogramWithContext{} +var _ metricWithExemplar = &HistogramWithContext{} + +// WithContext allows a Counter to bind to a context. func (h *Histogram) WithContext(ctx context.Context) *HistogramWithContext { + // Return reference to a new histogram as modifying the existing one overrides the older context, + // and blocks with semaphores. So this is a better option, see: + // https://github.com/kubernetes/kubernetes/pull/128575#discussion_r1829509144. return &HistogramWithContext{ ctx: ctx, Histogram: h, } } +// withExemplar attaches an exemplar to the metric. +func (e *HistogramWithContext) withExemplar(v float64) { + if m, ok := e.Histogram.ObserverMetric.(prometheus.ExemplarObserver); ok { + maybeSpanCtx := trace.SpanContextFromContext(e.ctx) + if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { + exemplarLabels := prometheus.Labels{ + "trace_id": maybeSpanCtx.TraceID().String(), + "span_id": maybeSpanCtx.SpanID().String(), + } + m.ObserveWithExemplar(v, exemplarLabels) + return + } + } + + e.ObserverMetric.Observe(v) +} + +func (e *HistogramWithContext) Observe(v float64) { + e.withExemplar(v) +} + // HistogramVec is the internal representation of our wrapping struct around prometheus // histogramVecs. type HistogramVec struct { @@ -132,12 +151,13 @@ type HistogramVec struct { originalLabels []string } -// NewHistogramVec returns an object which satisfies kubeCollector and wraps the -// prometheus.HistogramVec object. However, the object returned will not measure -// anything unless the collector is first registered, since the metric is lazily instantiated, -// and only members extracted after -// registration will actually measure anything. +// The implementation of kubeCollector is expected for collector registration. +// HistogramVec implements the kubeCollector interface, and not ObserverMetric interface. +var _ kubeCollector = &HistogramVec{} +// NewHistogramVec returns an object which satisfies kubeCollector and wraps the prometheus.HistogramVec object. +// However, the object returned will not measure anything unless the collector is first registered, since the metric is +// lazily instantiated, and only members extracted after registration will actually measure anything. func NewHistogramVec(opts *HistogramOpts, labels []string) *HistogramVec { opts.StabilityLevel.setDefaults() @@ -260,51 +280,10 @@ func (v *HistogramVec) ResetLabelAllowLists() { v.LabelValueAllowLists = nil } -// WithContext returns wrapped HistogramVec with context -func (v *HistogramVec) WithContext(ctx context.Context) *HistogramVecWithContext { - return &HistogramVecWithContext{ - ctx: ctx, - HistogramVec: v, - } -} - -// HistogramVecWithContext is the wrapper of HistogramVec with context. -type HistogramVecWithContext struct { - *HistogramVec - ctx context.Context -} - -func (h *HistogramVecWithContextWithObserver) Observe(v float64) { - h.withExemplar(v) -} - -func (h *HistogramVecWithContextWithObserver) withExemplar(v float64) { - if m, ok := h.observer.(prometheus.ExemplarObserver); ok { - maybeSpanCtx := trace.SpanContextFromContext(h.HistogramVecWithContext.ctx) - if maybeSpanCtx.IsValid() && maybeSpanCtx.IsSampled() { - m.ObserveWithExemplar(v, prometheus.Labels{ - "trace_id": maybeSpanCtx.TraceID().String(), - "span_id": maybeSpanCtx.SpanID().String(), - }) - return - } - } - - h.observer.Observe(v) -} - -// WithLabelValues is the wrapper of HistogramVec.WithLabelValues. -func (vc *HistogramVecWithContext) WithLabelValues(lvs ...string) *HistogramVecWithContextWithObserver { - return &HistogramVecWithContextWithObserver{ - HistogramVecWithContext: vc, - observer: vc.HistogramVec.WithLabelValues(lvs...), - } -} - -// With is the wrapper of HistogramVec.With. -func (vc *HistogramVecWithContext) With(labels map[string]string) *HistogramVecWithContextWithObserver { - return &HistogramVecWithContextWithObserver{ - HistogramVecWithContext: vc, - observer: vc.HistogramVec.With(labels), - } +// WithContext is a no-op for now, users should still attach this to vectors for seamless future API upgrades (which rely on the context). +// This is done to keep extensions (such as exemplars) on the counter and not its derivatives. +// Note that there are no actual uses for this in the codebase except for chaining it with `WithLabelValues`, which makes no use of the context. +// Furthermore, Prometheus, which is upstream from this library, does not support contextual vectors, so we don't want to diverge. +func (v *HistogramVec) WithContext(_ context.Context) *HistogramVec { + return v } diff --git a/staging/src/k8s.io/component-base/metrics/histogram_test.go b/staging/src/k8s.io/component-base/metrics/histogram_test.go index 3cf4e703912ea..3b8e9affb4434 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/histogram_test.go @@ -490,46 +490,39 @@ func TestHistogramWithExemplar(t *testing.T) { } func TestHistogramVecWithExemplar(t *testing.T) { - // Create context. - traceID := trace.TraceID([]byte("trace-0000-xxxxx")) - spanID := trace.SpanID([]byte("span-0000-xxxxx")) - ctxForSpanCtx := trace.ContextWithSpanContext(context.Background(), trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceID, - SpanID: spanID, - TraceFlags: trace.FlagsSampled, - })) - value := float64(10) - - // Create contextual histogram. - histogramVec := NewHistogramVec(&HistogramOpts{ - Name: "histogram_exemplar_test", - Help: "helpless", - Buckets: []float64{100}, - }, []string{"group"}) - h := histogramVec.WithContext(ctxForSpanCtx) - - // Register histogram. registry := newKubeRegistry(apimachineryversion.Info{ Major: "1", Minor: "15", GitVersion: "v1.15.0-alpha-1.12345", }) + histogramVec := NewHistogramVec(&HistogramOpts{ + Name: "histogram_exemplar_test", + Help: "histogram help", + Namespace: "namespace", + Subsystem: "subsystem", + Buckets: []float64{100}, + }, []string{"group"}) registry.MustRegister(histogramVec) - // Call underlying exemplar methods. + // no-op, but this shouldn't panic. + h := histogramVec.WithContext(nil) + + value := float64(1) h.WithLabelValues("foo").Observe(value) + h.WithLabelValues("foo").(prometheus.ExemplarObserver).ObserveWithExemplar(100, prometheus.Labels{ + "exemplar_label": "42", + }) - // Gather. mfs, err := registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 1 { - t.Fatalf("Got %v metric families, Want: 1 metric family", len(mfs)) - } + require.NoError(t, err, "Gather failed %v", err) - // Verify metric type. mf := mfs[0] + + assert.Lenf(t, mfs, 1, "Got %v metric families, Want: 1 metric family", len(mfs)) + assert.Equal(t, BuildFQName("namespace", "subsystem", "histogram_exemplar_test"), *mf.Name) + assert.Equal(t, "[ALPHA] histogram help", mf.GetHelp()) + assert.Lenf(t, mf.GetMetric(), 1, "Got %v metrics, wanted 1 as the count", len(mf.GetMetric())) + var m *dto.Metric switch mf.GetType() { case dto.MetricType_HISTOGRAM: @@ -538,124 +531,21 @@ func TestHistogramVecWithExemplar(t *testing.T) { t.Fatalf("Got %v metric type, Want: %v metric type", mf.GetType(), dto.MetricType_HISTOGRAM) } - // Verify value. - want := value - got := m.GetHistogram().GetSampleSum() - if got != want { - t.Fatalf("Got %f, wanted %f as the count", got, want) - } - - // Verify exemplars. - buckets := m.GetHistogram().GetBucket() - if len(buckets) == 0 { - t.Fatalf("Got 0 buckets, wanted 1") - } - e := buckets[0].GetExemplar() - if e == nil { - t.Fatalf("Got nil exemplar, wanted an exemplar") - } - eLabels := e.GetLabel() - if eLabels == nil { - t.Fatalf("Got nil exemplar label, wanted an exemplar label") - } - if len(eLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(eLabels)) - } - for _, l := range eLabels { - switch *l.Name { - case "trace_id": - if *l.Value != traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, traceID.String()) - } - case "span_id": - if *l.Value != spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) + var labelValue string + for _, l := range m.GetLabel() { + if *l.Name == "group" { + labelValue = *l.Value } } - // Verify that all contextual histogram calls are exclusive. - contextualHistogramVec := NewHistogramVec(&HistogramOpts{ - Name: "contextual_histogram_vec", - Help: "helpless", - Buckets: []float64{100}, - }, []string{"group"}) - traceIDa := trace.TraceID([]byte("trace-0000-aaaaa")) - spanIDa := trace.SpanID([]byte("span-0000-aaaaa")) - contextualHistogramVecA := contextualHistogramVec.WithContext(trace.ContextWithSpanContext(context.Background(), - trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceIDa, - SpanID: spanIDa, - TraceFlags: trace.FlagsSampled, - }), - )) - traceIDb := trace.TraceID([]byte("trace-0000-bbbbb")) - spanIDb := trace.SpanID([]byte("span-0000-bbbbb")) - contextualHistogramVecB := contextualHistogramVec.WithContext(trace.ContextWithSpanContext(context.Background(), - trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceIDb, - SpanID: spanIDb, - TraceFlags: trace.FlagsSampled, - }), - )) - runs := []struct { - spanID trace.SpanID - traceID trace.TraceID - contextualHistogramVec *HistogramVecWithContext - }{ - { - spanID: spanIDa, - traceID: traceIDa, - contextualHistogramVec: contextualHistogramVecA, - }, { - spanID: spanIDb, - traceID: traceIDb, - contextualHistogramVec: contextualHistogramVecB, - }, - } - for _, run := range runs { - registry.MustRegister(run.contextualHistogramVec) - run.contextualHistogramVec.WithLabelValues("foo").Observe(value) + gotValue := m.GetHistogram().GetSampleSum() + gotExemplarValue := m.GetHistogram().GetBucket()[0].GetExemplar().GetValue() + gotExemplarLabelName := m.GetHistogram().GetBucket()[0].GetExemplar().GetLabel()[0].GetName() + gotExemplarLabelValue := m.GetHistogram().GetBucket()[0].GetExemplar().GetLabel()[0].GetValue() - mfs, err = registry.Gather() - if err != nil { - t.Fatalf("Gather failed %v", err) - } - if len(mfs) != 2 { - t.Fatalf("Got %v metric families, Want: 2 metric families", len(mfs)) - } - - dtoMetric := mfs[0].GetMetric()[0] - dtoMetricBuckets := dtoMetric.GetHistogram().GetBucket() - if len(dtoMetricBuckets) == 0 { - t.Fatalf("Got nil buckets") - } - dtoMetricBucketsExemplar := dtoMetricBuckets[0].GetExemplar() - if dtoMetricBucketsExemplar == nil { - t.Fatalf("Got nil exemplar") - } - - dtoMetricLabels := dtoMetricBucketsExemplar.GetLabel() - if len(dtoMetricLabels) != 2 { - t.Fatalf("Got %v exemplar labels, wanted 2 exemplar labels", len(dtoMetricLabels)) - } - for _, l := range dtoMetricLabels { - switch *l.Name { - case "trace_id": - if *l.Value != run.traceID.String() { - t.Fatalf("Got %s as traceID, wanted %s", *l.Value, run.traceID.String()) - } - case "span_id": - if *l.Value != run.spanID.String() { - t.Fatalf("Got %s as spanID, wanted %s", *l.Value, run.spanID.String()) - } - default: - t.Fatalf("Got unexpected label %s", *l.Name) - } - } - - registry.Unregister(run.contextualHistogramVec) - } + assert.Equalf(t, "foo", labelValue, "Got %v, wanted %v as the label values", labelValue, "foo") + assert.Equalf(t, 101, int(gotValue), "Got %v, wanted %v as the count while setting group to %v", gotValue, 101, labelValue) + assert.Equalf(t, 100, int(gotExemplarValue), "Got %v, wanted %v as the exemplar value while setting group to %v", gotExemplarValue, 100, labelValue) + assert.Equalf(t, "exemplar_label", gotExemplarLabelName, "Got %v, wanted %v as the exemplar label name", gotExemplarLabelName, "exemplar_label") + assert.Equalf(t, "42", gotExemplarLabelValue, "Got %v, wanted %v as the exemplar label value", gotExemplarLabelValue, "42") }