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

Implement methods on Registry for managing prefix and common labels #400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexforster
Copy link

This allows users to set the registry's prefix or common labels when using the default registry.

}

/// `labels` returns a copy of the common labels for the registry, if any.
pub fn labels(&self) -> Option<HashMap<String, String>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in prefix() above, can we return a reference instead (and let the consumers clone if they want)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the borrow would outlive the function call.

@lucab
Copy link
Member

lucab commented Aug 10, 2021

Thanks for the PR! In order to merge, this will need a rebase and a Signed-off-by on each commit.

I think we'd need a bit more context behind this. I do like the getters (minus the clone part), but I'm not thrilled by the set/clean logic. I won't expect people to use the registry that way, and the fact that those methods internally lock will likely become a bad performance surprise for many people.
I'd prefer sticking to the "set options on build" pattern and only keep the getters here.

@polachok
Copy link

I think the problem this PR is inspired by is that it's register_* macros use default registry and creating metrics with non-default registries is very verbose. At least that's what I found in my use and this PR helps a lot, I can just add labels to the default registry and use macros as usual.

Disclaimer: I'm not the author, they may have another use case.

@lucab
Copy link
Member

lucab commented Aug 11, 2021

@polachok thanks, that was useful feedback! Next release should come with #396 which should hopefully improve that UX aspect, but taking a different path from this PR.

That said, I'm still intimately not in favor of doing this kind of registry mutation at runtime. It's a bazooka with side-effects at distance, which can easily turn into a very large footgun.

@alexforster
Copy link
Author

I think we'd need a bit more context behind this.

Right, I think based on your feedback that I wasn't clear on the use-case. Specifically, this is intended to make it possible to manipulate the set of default labels on the default registry. I do not believe that this adds any useful functionality for custom registries. However, I think custom registries are probably a very niche use-case anyway:

  • I've never had a need to create more than one registry in any codebase. I can't even think of a hypothetical scenario where I'd want to.
  • Creating a custom registry and passing it around everywhere is a huge chore. Metrics are everywhere in a typical codebase, so the registry object ends up infecting every corner of a system.

Because of this, I always use the default registry, and I bet 99% of your users do too.

I do like the getters (minus the clone part), but I'm not thrilled by the set/clean logic.

Keeping getters and removing setters doesn't add any meaningful functionality imo. The functionality I'm trying to add here is the ability to set default labels on the default registry.

I won't expect people to use the registry that way, and the fact that those methods internally lock will likely become a bad performance surprise for many people. I'd prefer sticking to the "set options on build" pattern and only keep the getters here.

Again, this is only for default labels on a Registry. Presumably default labels would only need to be configured once, not continuously.

This allows users to set the registry's prefix or common labels when using the default registry.

Signed-off-by: Alex Forster <[email protected]>
@alexforster
Copy link
Author

I've rebased and added a Signed-off-by

@adeschamps
Copy link

I thought I was going to need this feature, and was about to use this branch via a [patch.crates-io] entry. I wanted to expose a build version and other information that's static over the lifetime of the process. I ended up exposing it via a single gauge metric, as described in the article Exposing the software version to Prometheus instead (which I discovered via a recent blog post from the autometrics crate).

I do have a use case for multiple registries though -- we have an application that we deliver to users, and also operate as a hosted service. Some metrics measure implementation details or unstable features, so we initially add them to the default registry (which is only exposed in internal builds) and later switch them to a different registry when we're sure that they're useful for users and unlikely to be changed.

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.

4 participants