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

Metrics should be able to accept a prometheus registerer parameter #2047

Closed
Tracked by #2062
MarcoPolo opened this issue Feb 3, 2023 · 4 comments · Fixed by #2116
Closed
Tracked by #2062

Metrics should be able to accept a prometheus registerer parameter #2047

MarcoPolo opened this issue Feb 3, 2023 · 4 comments · Fixed by #2116
Labels
effort/hours Estimated to take one or several hours exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue

Comments

@MarcoPolo
Copy link
Collaborator

Instead of always using the default registerer.

@sukunrt
Copy link
Member

sukunrt commented Feb 11, 2023

Should I add An option to create a metrics tracer with a given registrar like done in eventbus metrics?
https://github.com/libp2p/go-libp2p/blob/master/p2p/host/eventbus/basic_metrics.go#L96-L100

func MustRegisterWith(reg prometheus.Registerer) MetricsTracerOption {
	return func(s *metricsTracerSetting) {
		s.reg = reg
	}
}

@marten-seemann
Copy link
Contributor

I'd be in favor of renaming the option. MustRegisterWith sounds like it's a global function (which it isn't really, it's an option), and it also sounds like it could panic. Better: WithRegisterer.

If I understand correctly, when allowing multiple prometheus.Registerers, we need to be careful with the sync.Once. We need to register once with every Registerer.

We probably need to introduce some kind of metrics helper: A (thread-safe) set to keep track of which Registerers we've already registered with.

@marten-seemann marten-seemann added help wanted Seeking public contribution on this issue exp/novice Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours labels Feb 11, 2023
@marten-seemann marten-seemann mentioned this issue Feb 11, 2023
25 tasks
@sukunrt
Copy link
Member

sukunrt commented Feb 12, 2023

Agreed on WithRegisterer.
Why do we need to register just once? The prometheus Registerer interface has a Register method which would give an error AlreadyRegisteredError on reregistration. We can ignore this error and panic/propagate other errors when doing registerer.Register. Is this good enough?

@marten-seemann
Copy link
Contributor

Why do we need to register just once? The prometheus Registerer interface has a Register method which would give an error AlreadyRegisteredError on reregistration. We can ignore this error and panic/propagate other errors when doing registerer.Register. Is this good enough?

Looks like I didn't look closely enough at the Prometheus API. This is even better! Catching AlreadyRegisteredError and panicking for all others sounds reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants