From 753a259e20233369ca3aff0682428150c9b0e9b6 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 10 May 2017 19:49:36 +0200 Subject: [PATCH 1/2] Improve promhttp tests - Use local registry to avoid conflicts between tests. - Expose https://github.com/prometheus/client_golang/issues/299 by using ConstLabels in a test. - Improve example: Buckets and help string must be consistent, even if the former is not enforced as of now, but see https://github.com/prometheus/client_golang/issues/222 --- .../promhttp/instrument_client_1_8_test.go | 4 +- prometheus/promhttp/instrument_server_test.go | 45 +++++++++---------- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/prometheus/promhttp/instrument_client_1_8_test.go b/prometheus/promhttp/instrument_client_1_8_test.go index 235a3d2..7e3f522 100644 --- a/prometheus/promhttp/instrument_client_1_8_test.go +++ b/prometheus/promhttp/instrument_client_1_8_test.go @@ -28,6 +28,8 @@ func TestClientMiddlewareAPI(t *testing.T) { client := http.DefaultClient client.Timeout = 1 * time.Second + reg := prometheus.NewRegistry() + inFlightGauge := prometheus.NewGauge(prometheus.GaugeOpts{ Name: "client_in_flight_requests", Help: "A gauge of in-flight requests for the wrapped client.", @@ -68,7 +70,7 @@ func TestClientMiddlewareAPI(t *testing.T) { []string{"method"}, ) - prometheus.MustRegister(counter, tlsLatencyVec, dnsLatencyVec, histVec, inFlightGauge) + reg.MustRegister(counter, tlsLatencyVec, dnsLatencyVec, histVec, inFlightGauge) trace := &InstrumentTrace{ DNSStart: func(t float64) { diff --git a/prometheus/promhttp/instrument_server_test.go b/prometheus/promhttp/instrument_server_test.go index 24668df..ca9f4bf 100644 --- a/prometheus/promhttp/instrument_server_test.go +++ b/prometheus/promhttp/instrument_server_test.go @@ -23,6 +23,8 @@ import ( ) func TestMiddlewareAPI(t *testing.T) { + reg := prometheus.NewRegistry() + inFlightGauge := prometheus.NewGauge(prometheus.GaugeOpts{ Name: "in_flight_requests", Help: "A gauge of requests currently being served by the wrapped handler.", @@ -38,9 +40,10 @@ func TestMiddlewareAPI(t *testing.T) { histVec := prometheus.NewHistogramVec( prometheus.HistogramOpts{ - Name: "response_duration_seconds", - Help: "A histogram of request latencies.", - Buckets: prometheus.DefBuckets, + Name: "response_duration_seconds", + Help: "A histogram of request latencies.", + Buckets: prometheus.DefBuckets, + ConstLabels: prometheus.Labels{"handler": "api"}, }, []string{"method"}, ) @@ -58,7 +61,7 @@ func TestMiddlewareAPI(t *testing.T) { w.Write([]byte("OK")) }) - prometheus.MustRegister(inFlightGauge, counter, histVec, responseSize) + reg.MustRegister(inFlightGauge, counter, histVec, responseSize) chain := InstrumentHandlerInFlight(inFlightGauge, InstrumentHandlerCounter(counter, @@ -87,29 +90,25 @@ func ExampleInstrumentHandlerDuration() { []string{"code", "method"}, ) - // pushVec is partitioned by the HTTP method and uses custom buckets based on - // the expected request duration. It uses ConstLabels to set a handler label - // marking pushVec as tracking the durations for pushes. + // pushVec and pullVec are partitioned by the HTTP method and use custom + // buckets based on the expected request duration. ConstLabels are used + // to set a handler label to mark pushVec as tracking the durations for + // pushes and pullVec as tracking the durations for pulls. Note that + // Name, Help, and Buckets need to be the same for consistency, so we + // use the same HistogramOpts after just modifying the ConstLabels. + histogramOpts := prometheus.HistogramOpts{ + Name: "request_duration_seconds", + Help: "A histogram of latencies for requests.", + Buckets: []float64{.25, .5, 1, 2.5, 5, 10}, + ConstLabels: prometheus.Labels{"handler": "push"}, + } pushVec := prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Name: "request_duration_seconds", - Help: "A histogram of latencies for requests to the push handler.", - Buckets: []float64{.25, .5, 1, 2.5, 5, 10}, - ConstLabels: prometheus.Labels{"handler": "push"}, - }, + histogramOpts, []string{"method"}, ) - - // pullVec is also partitioned by the HTTP method but uses custom buckets - // different from those for pushVec. It also has a different value for the - // constant "handler" label. + histogramOpts.ConstLabels = prometheus.Labels{"handler": "pull"} pullVec := prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Name: "request_duration_seconds", - Help: "A histogram of latencies for requests to the pull handler.", - Buckets: []float64{.005, .01, .025, .05}, - ConstLabels: prometheus.Labels{"handler": "pull"}, - }, + histogramOpts, []string{"method"}, ) From 023c31fd5923e9ce26df8f63e3c42785a618c157 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 10 May 2017 20:39:36 +0200 Subject: [PATCH 2/2] Fix handling of ConstLabels in checkLabels --- prometheus/promhttp/instrument_server.go | 50 +++++++++++++++--------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/prometheus/promhttp/instrument_server.go b/prometheus/promhttp/instrument_server.go index 193598d..1beab4b 100644 --- a/prometheus/promhttp/instrument_server.go +++ b/prometheus/promhttp/instrument_server.go @@ -27,6 +27,9 @@ import ( "github.com/prometheus/client_golang/prometheus" ) +// magicString is used for the hacky label test in checkLabels. Remove once fixed. +const magicString = "zZgWfBxLqvG8kc8IMv3POi2Bb0tZI3vAnBx+gBaFi9FyPzB/CzKUer1yufDa" + // InstrumentHandlerInFlight is a middleware that wraps the provided // http.Handler. It sets the provided prometheus.Gauge to the number of // requests currently handled by the wrapped http.Handler. @@ -191,37 +194,46 @@ func checkLabels(c prometheus.Collector) (code bool, method bool) { if _, err := prometheus.NewConstMetric(desc, prometheus.UntypedValue, 0); err == nil { return - } else if m, err := prometheus.NewConstMetric(desc, prometheus.UntypedValue, 0, ""); err == nil { + } + if m, err := prometheus.NewConstMetric(desc, prometheus.UntypedValue, 0, magicString); err == nil { if err := m.Write(&pm); err != nil { panic("error checking metric for labels") } - - name := *pm.Label[0].Name - if name == "code" { - code = true - } else if name == "method" { - method = true - } else { - panic("metric partitioned with non-supported labels") - } - return - } else if m, err := prometheus.NewConstMetric(desc, prometheus.UntypedValue, 0, "", ""); err == nil { - if err := m.Write(&pm); err != nil { - panic("error checking metric for labels") - } - for _, label := range pm.Label { - if *label.Name == "code" || *label.Name == "method" { + name, value := label.GetName(), label.GetValue() + if value != magicString { + continue + } + switch name { + case "code": + code = true + case "method": + method = true + default: + panic("metric partitioned with non-supported labels") + } + return + } + panic("previously set label not found – this must never happen") + } + if m, err := prometheus.NewConstMetric(desc, prometheus.UntypedValue, 0, magicString, magicString); err == nil { + if err := m.Write(&pm); err != nil { + panic("error checking metric for labels") + } + for _, label := range pm.Label { + name, value := label.GetName(), label.GetValue() + if value != magicString { + continue + } + if name == "code" || name == "method" { continue } panic("metric partitioned with non-supported labels") } - code = true method = true return } - panic("metric partitioned with non-supported labels") }