From 8a90ca328d70019732224571ed934a642e703c35 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Thu, 6 Nov 2025 10:54:42 +0200 Subject: [PATCH 1/2] Isolate TestNewHandlerFactory from other tests NewHandlerFactory is unfortunately registering its metrics in the global registry of Prometheus client library. This also leads to its tests leaking state between different test cases as they all access the same registry. This has been worked around by applying a namespace on the metric names of each test. Isolate the tests correctly by overriding the global registry with a test specific one, and recovering the original registry at the end of the test. This way each of the tests can access have their own isolated registry no longer need namespaces to avoid conflicts. As we're no longer implicitly testing the namespace option through using it for test isolation, we add an explicit test to assert the functionality works. --- metrics/handler_test.go | 46 ++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/metrics/handler_test.go b/metrics/handler_test.go index baaea111..a16c9522 100644 --- a/metrics/handler_test.go +++ b/metrics/handler_test.go @@ -15,23 +15,33 @@ import ( func TestNewHandlerFactory(t *testing.T) { var dynamicLabelKey struct{} - tests := []struct { + type TestCase struct { name string opts []HandlerFactoryOption // factory-level options handlerOpts []HandlerOption // handler-level options injectCtx func(r *http.Request) *http.Request // optional context injection wantLabels map[string]string // custom label key/values - }{ + namespace string + } + + tests := []TestCase{ { name: "basic", - opts: []HandlerFactoryOption{ - WithNamespace("basic"), - }, }, + func() TestCase { + const namespace = "custom_prefix" + return TestCase{ + name: "custom namespace", + opts: []HandlerFactoryOption{WithNamespace(namespace)}, + // Prometheus separates the namespace from the metric subsystem and name + // with an underscore. Add it here as well so the assertion correctly + // accounts for it. + namespace: namespace + "_", + } + }(), { name: "labels", opts: []HandlerFactoryOption{ - WithNamespace("labels"), WithLabels("label1", "label2"), }, handlerOpts: []HandlerOption{ @@ -42,7 +52,6 @@ func TestNewHandlerFactory(t *testing.T) { { name: "dynamic_labels", opts: []HandlerFactoryOption{ - WithNamespace("dynamic_labels"), WithLabels("color"), }, handlerOpts: []HandlerOption{ @@ -63,21 +72,18 @@ func TestNewHandlerFactory(t *testing.T) { { name: "request_duration_buckets", opts: []HandlerFactoryOption{ - WithNamespace("request_duration_buckets"), WithRequestDurationBuckets([]float64{1, 2, 3}), }, }, { name: "time_to_write_header_duration_buckets", opts: []HandlerFactoryOption{ - WithNamespace("time_to_write_header_duration_buckets"), WithTimeToWriteHeaderDurationBuckets([]float64{1, 2, 3}), }, }, { name: "byte_bucket_size_buckets", opts: []HandlerFactoryOption{ - WithNamespace("byte_bucket_size_buckets"), WithByteSizeBuckets([]float64{1, 2, 3}), }, }, @@ -85,6 +91,20 @@ func TestNewHandlerFactory(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // NewHandlerFactory unfortunately registers the metrics by default in the global registry which + // isn't something we can change without breaking backwards compatibility. Isolate these tests by + // overriding the global defaults with a custom registry, and revert back to the originals after + // the test. + originalRegisterer, originalGatherer := prometheus.DefaultRegisterer, prometheus.DefaultGatherer + defer func() { + prometheus.DefaultRegisterer = originalRegisterer + prometheus.DefaultGatherer = originalGatherer + }() + + defaultRegistry := prometheus.NewRegistry() + prometheus.DefaultRegisterer = defaultRegistry + prometheus.DefaultGatherer = defaultRegistry + factory := NewHandlerFactory(tt.opts...) require.NotNil(t, factory) @@ -120,8 +140,10 @@ func TestNewHandlerFactory(t *testing.T) { keyedMetrics[v.GetName()] = v } - // For each test, the namespace is "tt.name", so we expect e.g. "basic_http_requests_total" - ns := tt.name + "_http_" + // Add the possible custom namespace the test configures into the metric names. + // So we by default expect e.g. "basic_http_requests_total", and with a custom namespace + // e.g "namespace_basic_http_requests_total". + ns := tt.namespace + "http_" // Since each test calls 1 request, we expect: // - in-flight gauge => 0 after finishing -- GitLab From c448e786b54cc8eebb187d4d66d4fd91ab161429 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Thu, 6 Nov 2025 11:06:23 +0200 Subject: [PATCH 2/2] Add an option for providing a custom prometheus.Registerer NewHandlerFactory registers its metrics directly into the global registry for the Prometheus client library. This forces global state on applications that use the shared functionality from LabKit. Global state is discouraged as it makes testing difficult, the application code inflexible and more complex as it couples unrelated aspects of an application together. This commit adds an option to provide a custom registerer where the factory will register metrics. This way the application has an option to avoid the global state of the default registry. It would be better to leave the metric registration up to the application entirely but I've opted to stick with the existing pattern here and provide an option that fits in the existing API. --- metrics/handler.go | 14 ++++++++------ metrics/handler_factory_options.go | 15 ++++++++++++++- metrics/handler_test.go | 25 +++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/metrics/handler.go b/metrics/handler.go index 5af35433..769d63fc 100644 --- a/metrics/handler.go +++ b/metrics/handler.go @@ -92,12 +92,14 @@ func NewHandlerFactory(opts ...HandlerFactoryOption) HandlerFactory { ) ) - prometheus.MustRegister(httpInFlightRequests) - prometheus.MustRegister(httpRequestsTotal) - prometheus.MustRegister(httpRequestDurationSeconds) - prometheus.MustRegister(httpRequestSizeBytes) - prometheus.MustRegister(httpResponseSizeBytes) - prometheus.MustRegister(httpTimeToWriteHeaderSeconds) + factoryConfig.registerer.MustRegister( + httpInFlightRequests, + httpRequestsTotal, + httpRequestDurationSeconds, + httpRequestSizeBytes, + httpResponseSizeBytes, + httpTimeToWriteHeaderSeconds, + ) return func(next http.Handler, handlerOpts ...HandlerOption) http.Handler { handlerConfig, promOpts := applyHandlerOptions(handlerOpts) diff --git a/metrics/handler_factory_options.go b/metrics/handler_factory_options.go index 726d272d..1de2d49f 100644 --- a/metrics/handler_factory_options.go +++ b/metrics/handler_factory_options.go @@ -1,6 +1,9 @@ package metrics +import "github.com/prometheus/client_golang/prometheus" + type handlerFactoryConfig struct { + registerer prometheus.Registerer namespace string subsystem string requestDurationBuckets []float64 @@ -14,7 +17,8 @@ type HandlerFactoryOption func(*handlerFactoryConfig) func applyHandlerFactoryOptions(opts []HandlerFactoryOption) handlerFactoryConfig { config := handlerFactoryConfig{ - subsystem: "http", + registerer: prometheus.DefaultRegisterer, + subsystem: "http", requestDurationBuckets: []float64{ 0.005, /* 5ms */ 0.025, /* 25ms */ @@ -91,3 +95,12 @@ func WithByteSizeBuckets(buckets []float64) HandlerFactoryOption { config.byteSizeBuckets = buckets } } + +// WithRegisterer configures the Registerer the metrics will be registered with. +// The metrics are by default registered into prometheus.DefaultRegisterer if +// this option is not provided. +func WithRegisterer(registerer prometheus.Registerer) HandlerFactoryOption { + return func(config *handlerFactoryConfig) { + config.registerer = registerer + } +} diff --git a/metrics/handler_test.go b/metrics/handler_test.go index a16c9522..58ff0a4f 100644 --- a/metrics/handler_test.go +++ b/metrics/handler_test.go @@ -22,6 +22,7 @@ func TestNewHandlerFactory(t *testing.T) { injectCtx func(r *http.Request) *http.Request // optional context injection wantLabels map[string]string // custom label key/values namespace string + registry *prometheus.Registry } tests := []TestCase{ @@ -87,6 +88,15 @@ func TestNewHandlerFactory(t *testing.T) { WithByteSizeBuckets([]float64{1, 2, 3}), }, }, + func() TestCase { + registry := prometheus.NewRegistry() + + return TestCase{ + name: "injected registerer", + opts: []HandlerFactoryOption{WithRegisterer(registry)}, + registry: registry, + } + }(), } for _, tt := range tests { @@ -130,8 +140,19 @@ func TestNewHandlerFactory(t *testing.T) { handler.ServeHTTP(w, r) require.True(t, invoked, "handler was never invoked") - // Gather metrics from the default registry - metrics, err := prometheus.DefaultGatherer.Gather() + registry := defaultRegistry + if tt.registry != nil { + registry = tt.registry + + // If a registry was injected via options, no metrics should be registered + // into the default registry. + metrics, err := defaultRegistry.Gather() + require.NoError(t, err) + require.Empty(t, metrics) + } + + // Gather metrics from the registry + metrics, err := registry.Gather() require.NoError(t, err) require.NotEmpty(t, metrics) -- GitLab