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

Add a helper function to register all Collectors in a struct #712

Closed
wants to merge 1 commit into from

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Feb 12, 2020

This is reflection based, but registration is not time-critical.

Example where this is would come in handy:
https://github.com/prometheus/prometheus/blob/92f31c3ec290fa744e167c0cb2daebfabae6c82b/tsdb/compact.go#L132-L140

Note that the fields in compactorMetrics needed to be exported (but
not compactorMetrics itself).

@bwplotka that's what I was referring to. Do you think this would work for you?

This is reflection based, but registration is not time-critical.

Example where this is would come in handy:
https://github.com/prometheus/prometheus/blob/92f31c3ec290fa744e167c0cb2daebfabae6c82b/tsdb/compact.go#L132-L140

Note that the fields in compactorMetrics needed to be exported (but
not compactorMetrics itself).

Signed-off-by: beorn7 <beorn@grafana.com>
@bwplotka
Copy link
Member

Thanks for this 👍 ❤️

Hm. I think it's definitely better than what we have now, it's harder to miss a metric registration. I think having those fields exported might be surprising and reviewer/author can forget about that... Why we cannot register unexported ones? Seems like we can read those fields: https://stackoverflow.com/questions/17981651/is-there-any-way-to-access-private-fields-of-a-struct-from-another-package.

But overall, this is, still, an extra step that someone has to make (MustRegisterFieldsWith). And especially with just exported fields, it's still easy to forget to make them uppercase.

As a result, I am not convinced if it's better than just promauto with each NewCounter(reg, opts...), NewXXX.. etc. 🤔

But thanks for this example! We will definitely consider this in Thanos - maybe even start with this.

@beorn7
Copy link
Member Author

beorn7 commented Feb 12, 2020

Thanks for the thoughtful feedback.

Why we cannot register unexported ones?

That seems to be a deliberate measure by the Go runtime. You cannot turn unexported fields into an interface (see CanInterface) in the code, and thus cannot treat them as Collectors.

I tried yet another idea today. I'll send you another WIP PR immediately.

@beorn7
Copy link
Member Author

beorn7 commented Feb 12, 2020

One way out here would be to panic if we encounter an unexported Collector. But at the moment, I think my other idea will work better. Just a minute…

@beorn7
Copy link
Member Author

beorn7 commented Feb 13, 2020

I'll close this as #713 seems more like the way to go.

@beorn7 beorn7 closed this Feb 13, 2020
@beorn7 beorn7 deleted the beorn7/registry branch June 23, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants