Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registry Counter panic: duplicate metrics collector registration attempted #716

Closed
bigpigeon opened this issue Feb 24, 2020 · 6 comments
Closed
Labels

Comments

@bigpigeon
Copy link

client version

github.com/prometheus/client_golang v1.4.1

go.mod

module github.com/bigpigeon/Test/go/prometheus_example_http
go 1.13

require (
	github.com/prometheus/client_golang v1.4.1
	github.com/prometheus/common v0.9.1
)

Problem

when I Registry prometheus.Counter with prometheus must be panic with duplicate metrics collector registration attempted

source code


package main

import (
	"github.com/prometheus/common/expfmt"
	"net/http"
	"time"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/client_golang/prometheus/promauto"
)

func recordMetrics(opsProcessed prometheus.Counter) {
	go func() {
		for {
			opsProcessed.Inc()
			time.Sleep(2 * time.Second)
		}
	}()
}

func main() {

	opsProcessed := promauto.NewCounter(prometheus.CounterOpts{
		Namespace: "bigpigeon",
		Subsystem: "test",
		Name:      "xxx",
		Help:      "The total number of processed events",
	})

	prometheus.MustRegister(opsProcessed)

	recordMetrics(opsProcessed)

	mfs, err := prometheus.DefaultGatherer.Gather()
	if err != nil {
		panic(err)
	}
	http.HandleFunc("/metrics", func(writer http.ResponseWriter, request *http.Request) {
		for _, mf := range mfs {
			if mf.Help != nil && *mf.Help == "test histogram bucket" {
				if _, err := expfmt.MetricFamilyToText(writer, mf); err != nil {
					if err != nil {
						panic(err)
					}
				}
			}
		}
	})
	http.ListenAndServe(":9090", nil)
}
@bwplotka
Copy link
Member

bwplotka commented Feb 24, 2020

Hi 👋

This is expected, as you used promauto which automatically registers the metric. (:

You have few options (you can choose any of those three):

  1. Use prometheus package for creating metrics: prometheus.NewCounter(prometheus.CounterOpts{
  2. Don't register prometheus.MustRegister(opsProcessed). promauto already does that.
  3. Better: Use custom registry to have explicit control on what is registered and when:
reg := prometheus.NewRegistry()
opsProcessed := promauto.With(reg).NewCounter(prometheus.CounterOpts{
		Namespace: "bigpigeon",
		Subsystem: "test",
		Name:      "xxx",
		Help:      "The total number of processed events",
	})
// No need to register with prometheus.MustRegister(opsProcessed) anymore.

Let us know if you have more questions! 🤗 Otherwise closing (:

@beorn7
Copy link
Member

beorn7 commented Feb 24, 2020

It makes more sense to ask questions like this on the prometheus-users mailing list rather than in a GitHub issue. On the mailing list, more people are available to potentially respond to your question, and the whole community can benefit from the answers provided.

@1a1a11a
Copy link

1a1a11a commented Jan 17, 2021

@beorn7 GitHub is easier to find for someone not on the mailing list and benefits more people

@jeeftor
Copy link

jeeftor commented Feb 3, 2021

@1a1a11a - Fully agree with this

eh-am added a commit to eh-am/pyroscope that referenced this issue Aug 12, 2021
that's used to give more control and therefore
create unique registers per tests
avoiding the "duplicate metrics collector registration attempted" panic
message
see prometheus/client_golang#716 (comment)
eh-am added a commit to eh-am/pyroscope that referenced this issue Aug 12, 2021
that's used to give more control and therefore
create unique registers per tests
avoiding the "duplicate metrics collector registration attempted" panic
message
see prometheus/client_golang#716 (comment)
petethepig pushed a commit to grafana/pyroscope that referenced this issue Aug 23, 2021
* metrics: inline all counter references

remove all calls to metrics.Counter
and instead inline using the prometheus/promauto package

* metrics: inject prometheus register dependency

that's used to give more control and therefore
create unique registers per tests
avoiding the "duplicate metrics collector registration attempted" panic
message
see prometheus/client_golang#716 (comment)

* metrics: inline gauge calls

* metrics: inline all gauge calls in debug reporter

couple things:
1 - semantics have changed, ie the metrics are initialized to 0
even if we can't ever generate metrics data

so think in a dashboard a 0 value vs simply nothing showing up

2 - some of the metrics generated were dynamic, for example,
based on files in a directory
that has been hardcoded

* metrics: inline timing calls and remove metrics pkg

* metrics: bring back type assertions

* metrics: create basic playground to compare metrics

* metrics: convert timing metrics to nanoseconds

* metrics: improve metrics-comparison docker-compose

* metrics: fixes /benchmark

* metrics: fix broken test

* metrics: make missing metrics greppable
petethepig pushed a commit to grafana/pyroscope that referenced this issue Aug 23, 2021
* metrics: inline all counter references

remove all calls to metrics.Counter
and instead inline using the prometheus/promauto package

* metrics: inject prometheus register dependency

that's used to give more control and therefore
create unique registers per tests
avoiding the "duplicate metrics collector registration attempted" panic
message
see prometheus/client_golang#716 (comment)

* metrics: inline gauge calls

* metrics: inline all gauge calls in debug reporter

couple things:
1 - semantics have changed, ie the metrics are initialized to 0
even if we can't ever generate metrics data

so think in a dashboard a 0 value vs simply nothing showing up

2 - some of the metrics generated were dynamic, for example,
based on files in a directory
that has been hardcoded

* metrics: inline timing calls and remove metrics pkg

* metrics: bring back type assertions

* metrics: create basic playground to compare metrics

* metrics: convert timing metrics to nanoseconds

* metrics: improve metrics-comparison docker-compose

* metrics: fixes /benchmark

* metrics: fix broken test

* dashboard: initial version created with jsonnet

* metrics: add pyroscope_ suffix

* metrics: make missing metrics greppable

* metrics: prepend pyroscope_ to missing metrics

* metrics: refactor storage and cache counters

* metrics: rename pyroscope_disk and pyroscope_cache_size

* metrics: rename _count suffix to _total

* metrics: timers are now histograms

* metrics: refactor cache writes/misses metrics

* metrics: rename a bunch of metrics

* metrics: add dashboard row for go metrics

* metrics: cleanup dashboard

* metrics: add help text to benchmark metrics
@meetsubhojit
Copy link

@beorn7 GitHub is easier to find for someone not on the mailing list and benefits more people

and its google searchable

@clintonShopify
Copy link

Going to just add my notes in case it's helpful for anyone else. I think @bwplotka gave a solid answer but I'll add to it.

Bottom Line Upfront

Use prometheus client instead of promauto to handle fallible registry of new prometheus metrics.

  1. Sometimes one might have a metric already register. There is an example on how to handle already registered metrics that one can follow.

Why?

I was using promauto but as the notes in the code suggests:

This appears very handy. So why are these constructors locked away in a
separate package?

The main problem is that registration may fail, e.g. if a metric inconsistent
with or equal to the newly to be registered one is already registered.
Therefore, the Register method in the prometheus.Registerer interface returns
an error, and the same is the case for the top-level prometheus.Register
function that registers with the global registry. The prometheus package also
provides MustRegister versions for both. They panic if the registration
fails, and they clearly call this out by using the Must… idiom. Panicking is
problematic in this case because it doesn't just happen on input provided by
the caller that is invalid on its own. Things are a bit more subtle here:
Metric creation and registration tend to be spread widely over the
codebase. It can easily happen that an incompatible metric is added to an
unrelated part of the code, and suddenly code that used to work perfectly
fine starts to panic (provided that the registration of the newly added
metric happens before the registration of the previously existing
metric). This may come as an even bigger surprise with the global registry,
where simply importing another package can trigger a panic (if the newly
imported package registers metrics in its init function). At least, in the
prometheus package, creation of metrics and other collectors is separate from
registration. You first create the metric, and then you decide explicitly if
you want to register it with a local or the global registry, and if you want
to handle the error or risk a panic. With the constructors in the promauto
package, registration is automatic, and if it fails, it will always
panic. Furthermore, the constructors will often be called in the var section
of a file, which means that panicking will happen as a side effect of merely
importing a package.

A separate package allows conservative users to entirely ignore it. And
whoever wants to use it will do so explicitly, with an opportunity to read
this warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants