From 6cd66da21fd6be4d61ed71c2eb3c8eb9696a9f51 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Sun, 15 Nov 2026 10:48:37 +0100 Subject: [PATCH] fix(telemetry): seconds bucket bounds on every *_seconds histogram The 13 duration histograms were created with no bucket bounds, and telemetry.Setup's provider has no View, so the SDK's defaults applied: [0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000]. Those bounds suit milliseconds. On a seconds metric, everything under 5 s lands in one bucket, so a quantile is only a coarse upper bound. observability found it on krateo-057 (2026-11-15): reconcile means of 2.8-7.9 s all reported their p95 in the 75-250 s buckets. Each histogram now carries explicit bounds, from 1 ms to 10000 s: the list the composition-dynamic-controller's own histograms already use, so the two families compare bucket for bucket. The bounds are set on the instrument (metric.WithExplicitBucketBoundaries), so they apply under any provider, including a caller's own via NewWithMeter. A provider View still overrides them. Names and units are unchanged; only the bucket boundaries a consumer sees change. Test: every collected histogram (13, counted) carries the bounds, and a 0.3 s and a 2.8 s reconcile land in two different buckets. Mutants, each killed (3/3): reconcile.duration on the defaults; oldest_item_age on the defaults; the old millisecond list. Unit tests (18 packages) and vet under every tag pass. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01N8HNbBzRpE6rUjMRzosAjG --- pkg/telemetry/metrics.go | 43 +++++++++++++++++-------- pkg/telemetry/metrics_test.go | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/pkg/telemetry/metrics.go b/pkg/telemetry/metrics.go index 13c7e8a..1f30386 100644 --- a/pkg/telemetry/metrics.go +++ b/pkg/telemetry/metrics.go @@ -200,6 +200,23 @@ func NewWithMeter(meter metric.Meter, log logging.Logger, deploymentName string) return newMetrics(meter, log, deploymentName) } +// secondsBoundaries are the bucket upper bounds of every *_seconds histogram this package records. +// Without them the SDK applies its default bounds, [0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, +// 2500, 5000, 7500, 10000], which were chosen for MILLISECONDS: on a seconds metric every operation +// under 5 s falls in one bucket, so a quantile reads only as a coarse upper bound (observability, +// krateo-057, 2026-11-15: reconcile means of 2.8-7.9 s all reported p95 in the 75-250 s buckets). +// These are the bounds the composition-dynamic-controller's own histograms already use (1 ms to +// 10000 s), so the two families compare bucket for bucket. They are set on the instrument, so they +// apply under any provider, including a caller's own (NewWithMeter); a provider View still overrides +// them. +var secondsBoundaries = []float64{ + 0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, + 2.5, 5, 10, 25, 50, 100, 250, 500, 1000, + 2500, 5000, 10000, +} + +var secondsBuckets = metric.WithExplicitBucketBoundaries(secondsBoundaries...) + func newMetrics(meter metric.Meter, log logging.Logger, deploymentName string) (*Metrics, error) { var err error m := &Metrics{meters: &meters{ @@ -215,25 +232,25 @@ func newMetrics(meter metric.Meter, log logging.Logger, deploymentName string) ( if m.startupFailure, err = meter.Int64Counter("unstructured_runtime.startup.failure"); err != nil { return nil, err } - if m.getDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.get.duration_seconds"); err != nil { + if m.getDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.get.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.getFailure, err = meter.Int64Counter("unstructured_runtime.reconcile.get.failure"); err != nil { return nil, err } - if m.reconcileDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.duration_seconds"); err != nil { + if m.reconcileDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.queueDepth, err = meter.Int64UpDownCounter("unstructured_runtime.reconcile.queue.depth"); err != nil { return nil, err } - if m.queueWaitDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.wait.duration_seconds"); err != nil { + if m.queueWaitDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.wait.duration_seconds", secondsBuckets); err != nil { return nil, err } - if m.queueOldestItemAge, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.oldest_item_age_seconds"); err != nil { + if m.queueOldestItemAge, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.oldest_item_age_seconds", secondsBuckets); err != nil { return nil, err } - if m.queueWorkDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.work.duration_seconds"); err != nil { + if m.queueWorkDuration, err = meter.Float64Histogram("unstructured_runtime.reconcile.queue.work.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.reconcileInFlight, err = meter.Int64ObservableGauge("unstructured_runtime.reconcile.in_flight"); err != nil { @@ -257,49 +274,49 @@ func newMetrics(meter metric.Meter, log logging.Logger, deploymentName string) ( if m.queueRequeueTotal, err = meter.Int64Counter("unstructured_runtime.reconcile.queue.requeues"); err != nil { return nil, err } - if m.connectDuration, err = meter.Float64Histogram("unstructured_runtime.external.connect.duration_seconds"); err != nil { + if m.connectDuration, err = meter.Float64Histogram("unstructured_runtime.external.connect.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.connectFailure, err = meter.Int64Counter("unstructured_runtime.external.connect.failure"); err != nil { return nil, err } - if m.observeDuration, err = meter.Float64Histogram("unstructured_runtime.external.observe.duration_seconds"); err != nil { + if m.observeDuration, err = meter.Float64Histogram("unstructured_runtime.external.observe.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.observeFailure, err = meter.Int64Counter("unstructured_runtime.external.observe.failure"); err != nil { return nil, err } - if m.createDuration, err = meter.Float64Histogram("unstructured_runtime.external.create.duration_seconds"); err != nil { + if m.createDuration, err = meter.Float64Histogram("unstructured_runtime.external.create.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.createFailure, err = meter.Int64Counter("unstructured_runtime.external.create.failure"); err != nil { return nil, err } - if m.updateDuration, err = meter.Float64Histogram("unstructured_runtime.external.update.duration_seconds"); err != nil { + if m.updateDuration, err = meter.Float64Histogram("unstructured_runtime.external.update.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.updateFailure, err = meter.Int64Counter("unstructured_runtime.external.update.failure"); err != nil { return nil, err } - if m.deleteDuration, err = meter.Float64Histogram("unstructured_runtime.external.delete.duration_seconds"); err != nil { + if m.deleteDuration, err = meter.Float64Histogram("unstructured_runtime.external.delete.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.deleteFailure, err = meter.Int64Counter("unstructured_runtime.external.delete.failure"); err != nil { return nil, err } - if m.finalizerAddDuration, err = meter.Float64Histogram("unstructured_runtime.finalizer.add.duration_seconds"); err != nil { + if m.finalizerAddDuration, err = meter.Float64Histogram("unstructured_runtime.finalizer.add.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.finalizerAddFailure, err = meter.Int64Counter("unstructured_runtime.finalizer.add.failure"); err != nil { return nil, err } - if m.finalizerRemoveDuration, err = meter.Float64Histogram("unstructured_runtime.finalizer.remove.duration_seconds"); err != nil { + if m.finalizerRemoveDuration, err = meter.Float64Histogram("unstructured_runtime.finalizer.remove.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.finalizerRemoveFailure, err = meter.Int64Counter("unstructured_runtime.finalizer.remove.failure"); err != nil { return nil, err } - if m.statusUpdateDuration, err = meter.Float64Histogram("unstructured_runtime.status.update.duration_seconds"); err != nil { + if m.statusUpdateDuration, err = meter.Float64Histogram("unstructured_runtime.status.update.duration_seconds", secondsBuckets); err != nil { return nil, err } if m.statusUpdateFailure, err = meter.Int64Counter("unstructured_runtime.status.update.failure"); err != nil { diff --git a/pkg/telemetry/metrics_test.go b/pkg/telemetry/metrics_test.go index 7b01494..42a87a8 100644 --- a/pkg/telemetry/metrics_test.go +++ b/pkg/telemetry/metrics_test.go @@ -2,6 +2,7 @@ package telemetry import ( "context" + "fmt" "testing" "time" @@ -128,3 +129,61 @@ func hasMetric(rm metricdata.ResourceMetrics, name string) bool { return false } + +// Every *_seconds histogram carries secondsBoundaries, not the SDK's millisecond-style defaults, and +// a sub-second and a multi-second reconcile land in different buckets (under the defaults both fall +// in [0, 5]). +func TestSecondsHistogramsHaveSecondsBuckets(t *testing.T) { + reader := metric.NewManualReader() + provider := metric.NewMeterProvider(metric.WithReader(reader)) + ctx := context.Background() + defer func() { _ = provider.Shutdown(ctx) }() + m, err := newMetrics(provider.Meter("test"), logging.NewNopLogger(), "test-deployment") + if err != nil { + t.Fatal(err) + } + for _, rec := range []func(context.Context, time.Duration){ + m.RecordGetDuration, m.RecordQueueWaitDuration, m.RecordQueueOldestItemAge, m.RecordQueueWorkDuration, + m.RecordConnectDuration, m.RecordObserveDuration, m.RecordCreateDuration, m.RecordUpdateDuration, + m.RecordDeleteDuration, m.RecordFinalizerAddDuration, m.RecordFinalizerRemoveDuration, m.RecordStatusUpdateDuration, + } { + rec(ctx, 10*time.Millisecond) + } + m.RecordReconcileDuration(ctx, 300*time.Millisecond) + m.RecordReconcileDuration(ctx, 2800*time.Millisecond) + + var rm metricdata.ResourceMetrics + if err := reader.Collect(ctx, &rm); err != nil { + t.Fatal(err) + } + seen := 0 + for _, sm := range rm.ScopeMetrics { + for _, md := range sm.Metrics { + h, ok := md.Data.(metricdata.Histogram[float64]) + if !ok { + continue + } + seen++ + for _, dp := range h.DataPoints { + if fmt.Sprint(dp.Bounds) != fmt.Sprint(secondsBoundaries) { + t.Errorf("%s: bounds %v, want %v", md.Name, dp.Bounds, secondsBoundaries) + } + if md.Name != "unstructured_runtime.reconcile.duration_seconds" { + continue + } + nonEmpty := 0 + for _, c := range dp.BucketCounts { + if c > 0 { + nonEmpty++ + } + } + if nonEmpty != 2 { + t.Errorf("reconcile 0.3 s and 2.8 s: %d non-empty buckets %v, want 2", nonEmpty, dp.BucketCounts) + } + } + } + } + if seen != 13 { + t.Fatalf("histograms collected: %d, want 13 (every *_seconds instrument)", seen) + } +}