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

feat(registry): add with_prefix_and_labels constructor #147

Merged
merged 7 commits into from Jul 10, 2023

Conversation

popadi
Copy link
Contributor

@popadi popadi commented May 29, 2023

Added new constructor per discussion in #146.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Put simply there are two ways to create a registry:

  1. From scratch e.g. via Registry::with_prefix or <Registry as Default>::default.
  2. From a parent registry e.g. via Registry::sub_registry_with_prefix.

What do you think of changing this patch to follow (2) and not (1). It would be a convenience method for folks wanting a sub registry with a prefix and one or more labels. It would still be possible for folks that want (1) via Registry::default().sub_registry_with_prefix_and_labels(..).

@popadi
Copy link
Contributor Author

popadi commented Jun 5, 2023

What do you think of changing this patch to follow (2) and not (1). It would be a convenience method for folks wanting a sub registry with a prefix and one or more labels.

Is there any reason there is a separate Registry::with_prefix if Registry::sub_registry_with_prefix already exist then? The root registry with default labels might be not used that much, but it's definitely a usecase people might have.

It would still be possible for folks that want (1) via Registry::default().sub_registry_with_prefix_and_labels(..).

I think we might still want something similar to (1), at least that's my usecase (and that's why I made this pull request in the first place). If I really want ALL my metrics to have a prefix AND a bunch of labels, it seems a bit weird to simulate the root registry using a sub-registry.

Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
@popadi
Copy link
Contributor Author

popadi commented Jul 5, 2023

@mxinden any update/feedback on this?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here. Thank you for expanding on your use-case. Agreed that we should support it.

For the sake of consistency, what do you think about also adding Registry::with_labels?

Also can you bump the crate version in Cargo.toml to v0.21.2 and add a changelog entry in CHANGELOG.md?

Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
@popadi
Copy link
Contributor Author

popadi commented Jul 8, 2023

Sorry for the delay here. Thank you for expanding on your use-case. Agreed that we should support it.

For the sake of consistency, what do you think about also adding Registry::with_labels?

Also can you bump the crate version in Cargo.toml to v0.21.2 and add a changelog entry in CHANGELOG.md?

Done, added the extra method + updated changelog and bumped version.
Thanks for the feedback

Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
@mxinden mxinden changed the title Fix #146: Add with_prefix_and_labels constructor to Registry feat(registry): add with_prefix_and_labels constructor Jul 10, 2023
src/registry.rs Outdated Show resolved Hide resolved
src/registry.rs Outdated Show resolved Hide resolved
Signed-off-by: Max Inden <mail@max-inden.de>
Signed-off-by: Max Inden <mail@max-inden.de>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you for the continued work!

@mxinden mxinden merged commit 97d8de8 into prometheus:master Jul 10, 2023
11 checks passed
@mxinden
Copy link
Member

mxinden commented Jul 10, 2023

Published and tagged prometheus-client v0.21.2.

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