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

Provide helper to "describe by collect". #239

Closed
beorn7 opened this issue Sep 19, 2016 · 7 comments
Closed

Provide helper to "describe by collect". #239

beorn7 opened this issue Sep 19, 2016 · 7 comments
Assignees
Milestone

Comments

@beorn7
Copy link
Member

beorn7 commented Sep 19, 2016

If the metrics collected by a collector are using the same descriptors over the runtime of a binary, the Describe method can be implemented by calling Collect and then return all the Descs of the returned metrics.

There are a number of cornercases where this is not possible, so we cannot just do the above by default. However, it can be explicitly used by implementers of Collectors.

@beorn7 beorn7 added this to the v0.9 milestone Sep 19, 2016
@beorn7 beorn7 self-assigned this Sep 19, 2016
@brian-brazil
Copy link
Contributor

brian-brazil commented Sep 19, 2016

I think we can&should do the above by default, however it should be disabled by default on custom registries and there should be a way to bypass it on a per-collector basis (e.g. by implementing a no-op Desc).

@beorn7
Copy link
Member Author

beorn7 commented Sep 19, 2016

How would an implementation look like that does "describe by collect" by default?

@brian-brazil
Copy link
Contributor

For Go, you'd remove the current Desc from the interface and add a new interface with the Desc. Then based on which interfaces are implemented, you either can auto-collect or use the user-provided Desc.

@beorn7
Copy link
Member Author

beorn7 commented Sep 19, 2016

I see. Intuitively, I don't like the implicitness of it. Interface upgrades are a double-edged sword. In this case, it would save two lines of code for those that want to use the feature. I also think it's too easy to run into one of the cornercases to make it an implicit default. (Any metric vector won't work without initialization of at least one metric in it, as an example.)

@beorn7
Copy link
Member Author

beorn7 commented Sep 19, 2016

I suggest to go with a helper function for v0.9 to not change the interfaces right now.
Then we can gather some experience.
Based on the outcome, we can consider the interface change in v0.10, which will change many interfaces anyway and opens up the opportunity for those more invasive changes.

@brian-brazil
Copy link
Contributor

I imagine the cases where it won't be okay will be quite rare, so the wins in terms of filtering and ease of use will make up for it.

@beorn7
Copy link
Member Author

beorn7 commented Sep 5, 2018

Note: The suggested change of the interface to make DescribeByCollect implicit if no Describe method exists could still be considered for v0.10. (I'm kind of sceptical right now because it would not be "safe by default", i.e. somebody who naively implements a Collector without Describe would create something that seemingly works but then breaks later in surprising ways. But we can have that discussion when v0.10 is actually being worked on.)

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

No branches or pull requests

2 participants