From 2fee50beaa152342e8347c41edf2202c3bd5f858 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 25 Oct 2016 16:41:39 +0200 Subject: [PATCH] Remove deprecated features that are esay to replace That's the "soft" part of the deprecation: Everything that has been marked deprecated in v0.8 or earlier and is straight-forward to replace by a non-deprecated way, is removed here. Sadly, this does not include the HTTP part. We first need to provide a replacement for HTTP instrumentation (as planned for v0.8) to then remove the deprecated parts in v0.9. --- prometheus/counter.go | 9 ------- prometheus/http.go | 43 +++++++++++++++++++++++-------- prometheus/http_test.go | 42 ++++++++++++++++++++++++++---- prometheus/registry.go | 51 ------------------------------------- prometheus/registry_test.go | 23 +++++++++-------- 5 files changed, 82 insertions(+), 86 deletions(-) diff --git a/prometheus/counter.go b/prometheus/counter.go index ee37949..d0fdd5d 100644 --- a/prometheus/counter.go +++ b/prometheus/counter.go @@ -30,15 +30,6 @@ type Counter interface { Metric Collector - // Set is used to set the Counter to an arbitrary value. It is only used - // if you have to transfer a value from an external counter into this - // Prometheus metric. Do not use it for regular handling of a - // Prometheus counter (as it can be used to break the contract of - // monotonically increasing values). - // - // Deprecated: Use NewConstMetric to create a counter for an external - // value. A Counter should never be set. - Set(float64) // Inc increments the counter by 1. Inc() // Add adds the given value to the counter. It panics if the value is < diff --git a/prometheus/http.go b/prometheus/http.go index b2b0019..b8153e1 100644 --- a/prometheus/http.go +++ b/prometheus/http.go @@ -62,7 +62,7 @@ func giveBuf(buf *bytes.Buffer) { // // Deprecated: Please note the issues described in the doc comment of // InstrumentHandler. You might want to consider using promhttp.Handler instead -// (which is non instrumented). +// (which is not instrumented). func Handler() http.Handler { return InstrumentHandler("prometheus", UninstrumentedHandler()) } @@ -245,23 +245,46 @@ func InstrumentHandlerFuncWithOpts(opts SummaryOpts, handlerFunc func(http.Respo }, instLabels, ) + if err := Register(reqCnt); err != nil { + if are, ok := err.(AlreadyRegisteredError); ok { + reqCnt = are.ExistingCollector.(*CounterVec) + } else { + panic(err) + } + } opts.Name = "request_duration_microseconds" opts.Help = "The HTTP request latencies in microseconds." reqDur := NewSummary(opts) + if err := Register(reqDur); err != nil { + if are, ok := err.(AlreadyRegisteredError); ok { + reqDur = are.ExistingCollector.(Summary) + } else { + panic(err) + } + } opts.Name = "request_size_bytes" opts.Help = "The HTTP request sizes in bytes." reqSz := NewSummary(opts) + if err := Register(reqSz); err != nil { + if are, ok := err.(AlreadyRegisteredError); ok { + reqSz = are.ExistingCollector.(Summary) + } else { + panic(err) + } + } opts.Name = "response_size_bytes" opts.Help = "The HTTP response sizes in bytes." resSz := NewSummary(opts) - - regReqCnt := MustRegisterOrGet(reqCnt).(*CounterVec) - regReqDur := MustRegisterOrGet(reqDur).(Summary) - regReqSz := MustRegisterOrGet(reqSz).(Summary) - regResSz := MustRegisterOrGet(resSz).(Summary) + if err := Register(resSz); err != nil { + if are, ok := err.(AlreadyRegisteredError); ok { + resSz = are.ExistingCollector.(Summary) + } else { + panic(err) + } + } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { now := time.Now() @@ -285,10 +308,10 @@ func InstrumentHandlerFuncWithOpts(opts SummaryOpts, handlerFunc func(http.Respo method := sanitizeMethod(r.Method) code := sanitizeCode(delegate.status) - regReqCnt.WithLabelValues(method, code).Inc() - regReqDur.Observe(elapsed) - regResSz.Observe(float64(delegate.written)) - regReqSz.Observe(float64(<-out)) + reqCnt.WithLabelValues(method, code).Inc() + reqDur.Observe(elapsed) + resSz.Observe(float64(delegate.written)) + reqSz.Observe(float64(<-out)) }) } diff --git a/prometheus/http_test.go b/prometheus/http_test.go index ffe0418..3b7f939 100644 --- a/prometheus/http_test.go +++ b/prometheus/http_test.go @@ -46,7 +46,7 @@ func TestInstrumentHandler(t *testing.T) { ConstLabels: Labels{"handler": "test-handler"}, } - reqCnt := MustRegisterOrGet(NewCounterVec( + reqCnt := NewCounterVec( CounterOpts{ Namespace: opts.Namespace, Subsystem: opts.Subsystem, @@ -55,19 +55,51 @@ func TestInstrumentHandler(t *testing.T) { ConstLabels: opts.ConstLabels, }, instLabels, - )).(*CounterVec) + ) + err := Register(reqCnt) + if err == nil { + t.Fatal("expected reqCnt to be registered already") + } + if are, ok := err.(AlreadyRegisteredError); ok { + reqCnt = are.ExistingCollector.(*CounterVec) + } else { + t.Fatal("unexpected registration error:", err) + } opts.Name = "request_duration_microseconds" opts.Help = "The HTTP request latencies in microseconds." - reqDur := MustRegisterOrGet(NewSummary(opts)).(Summary) + reqDur := NewSummary(opts) + err = Register(reqDur) + if err == nil { + t.Fatal("expected reqDur to be registered already") + } + if are, ok := err.(AlreadyRegisteredError); ok { + reqDur = are.ExistingCollector.(Summary) + } else { + t.Fatal("unexpected registration error:", err) + } opts.Name = "request_size_bytes" opts.Help = "The HTTP request sizes in bytes." - MustRegisterOrGet(NewSummary(opts)) + reqSz := NewSummary(opts) + err = Register(reqSz) + if err == nil { + t.Fatal("expected reqSz to be registered already") + } + if _, ok := err.(AlreadyRegisteredError); !ok { + t.Fatal("unexpected registration error:", err) + } opts.Name = "response_size_bytes" opts.Help = "The HTTP response sizes in bytes." - MustRegisterOrGet(NewSummary(opts)) + resSz := NewSummary(opts) + err = Register(resSz) + if err == nil { + t.Fatal("expected resSz to be registered already") + } + if _, ok := err.(AlreadyRegisteredError); !ok { + t.Fatal("unexpected registration error:", err) + } reqCnt.Reset() diff --git a/prometheus/registry.go b/prometheus/registry.go index 405d6dd..78d5f19 100644 --- a/prometheus/registry.go +++ b/prometheus/registry.go @@ -152,38 +152,6 @@ func MustRegister(cs ...Collector) { DefaultRegisterer.MustRegister(cs...) } -// RegisterOrGet registers the provided Collector with the DefaultRegisterer and -// returns the Collector, unless an equal Collector was registered before, in -// which case that Collector is returned. -// -// Deprecated: RegisterOrGet is merely a convenience function for the -// implementation as described in the documentation for -// AlreadyRegisteredError. As the use case is relatively rare, this function -// will be removed in a future version of this package to clean up the -// namespace. -func RegisterOrGet(c Collector) (Collector, error) { - if err := Register(c); err != nil { - if are, ok := err.(AlreadyRegisteredError); ok { - return are.ExistingCollector, nil - } - return nil, err - } - return c, nil -} - -// MustRegisterOrGet behaves like RegisterOrGet but panics instead of returning -// an error. -// -// Deprecated: This is deprecated for the same reason RegisterOrGet is. See -// there for details. -func MustRegisterOrGet(c Collector) Collector { - c, err := RegisterOrGet(c) - if err != nil { - panic(err) - } - return c -} - // Unregister removes the registration of the provided Collector from the // DefaultRegisterer. // @@ -201,25 +169,6 @@ func (gf GathererFunc) Gather() ([]*dto.MetricFamily, error) { return gf() } -// SetMetricFamilyInjectionHook replaces the DefaultGatherer with one that -// gathers from the previous DefaultGatherers but then merges the MetricFamily -// protobufs returned from the provided hook function with the MetricFamily -// protobufs returned from the original DefaultGatherer. -// -// Deprecated: This function manipulates the DefaultGatherer variable. Consider -// the implications, i.e. don't do this concurrently with any uses of the -// DefaultGatherer. In the rare cases where you need to inject MetricFamily -// protobufs directly, it is recommended to use a custom Registry and combine it -// with a custom Gatherer using the Gatherers type (see -// there). SetMetricFamilyInjectionHook only exists for compatibility reasons -// with previous versions of this package. -func SetMetricFamilyInjectionHook(hook func() []*dto.MetricFamily) { - DefaultGatherer = Gatherers{ - DefaultGatherer, - GathererFunc(func() ([]*dto.MetricFamily, error) { return hook(), nil }), - } -} - // AlreadyRegisteredError is returned by the Register method if the Collector to // be registered has already been registered before, or a different Collector // that collects the same metrics has been registered before. Registration fails diff --git a/prometheus/registry_test.go b/prometheus/registry_test.go index 9dacb62..d016a15 100644 --- a/prometheus/registry_test.go +++ b/prometheus/registry_test.go @@ -526,20 +526,21 @@ func TestRegisterWithOrGet(t *testing.T) { }, []string{"foo", "bar"}, ) - if err := prometheus.Register(original); err != nil { + var err error + if err = prometheus.Register(original); err != nil { t.Fatal(err) } - if err := prometheus.Register(equalButNotSame); err == nil { + if err = prometheus.Register(equalButNotSame); err == nil { t.Fatal("expected error when registringe equal collector") } - existing, err := prometheus.RegisterOrGet(equalButNotSame) - if err != nil { - t.Fatal(err) - } - if existing != original { - t.Error("expected original collector but got something else") - } - if existing == equalButNotSame { - t.Error("expected original callector but got new one") + if are, ok := err.(prometheus.AlreadyRegisteredError); ok { + if are.ExistingCollector != original { + t.Error("expected original collector but got something else") + } + if are.ExistingCollector == equalButNotSame { + t.Error("expected original callector but got new one") + } + } else { + t.Error("unexpected error:", err) } }