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

src/metrics/family.rs: Add remove and clear methods, and tests #85

Merged

Conversation

phyber
Copy link
Contributor

@phyber phyber commented Aug 31, 2022

This PR adds Family::remove as discussed a little in #36 along with a basic test for the behaviour. We also add a Family::clear method as discussed in this PR.

The Family::remove method returns a bool to indicate if a label set was removed, which matches what the Go client does when removing its equivalent of a label set.

The Family::clear method does not return anything and always successfully clears the label sets.

@phyber phyber force-pushed the 20220831-add_family_remove_label_set branch 2 times, most recently from c467c47 to 35f0e3e Compare September 1, 2022 21:40
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.

Solid work! Thanks, especially for the doc comment and example!

According to the client library documentation there should be both a remove and a clear. I don't have a strong opinion for the latter. But given that is should be easy to add, what do you think of including it in this pull request?

Metrics with labels SHOULD support a remove() method with the same signature as labels() that will remove a Child from the metric no longer exporting it, and a clear() method that removes all Children from the metric. These invalidate caching of Children.

https://prometheus.io/docs/instrumenting/writing_clientlibs/#labels

src/metrics/family.rs Outdated Show resolved Hide resolved
src/metrics/family.rs Outdated Show resolved Hide resolved
@phyber
Copy link
Contributor Author

phyber commented Sep 4, 2022

I'd be happy to work on adding a clear method here as well. I might not have that before the end of this week though, as I'm away for a few days :)

@mxinden
Copy link
Member

mxinden commented Sep 5, 2022

No rush @phyber. Thanks for the help.

@phyber phyber force-pushed the 20220831-add_family_remove_label_set branch from f0acbd5 to 7fb4320 Compare September 9, 2022 19:28
@phyber
Copy link
Contributor Author

phyber commented Sep 9, 2022

Family::remove_label_sets has now been renamed to Family::remove, and a Family::clear method has been added.

The redundant Family::write private method was removed.

@phyber phyber changed the title src/metrics/family.rs: Add remove_label_set and test src/metrics/family.rs: Add remove and clear methods, and tests Sep 9, 2022
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.

Thanks for the follow-ups!

Just needs a bump of the patch version in Cargo.toml and a changelog entry in CHANGELOG.md.

@phyber phyber force-pushed the 20220831-add_family_remove_label_set branch from 2768a32 to 79ff924 Compare September 11, 2022 19:39
Signed-off-by: David O'Rourke <david.orourke@gmail.com>
Also remove redundant write method.

Signed-off-by: David O'Rourke <david.orourke@gmail.com>
Signed-off-by: David O'Rourke <david.orourke@gmail.com>
Signed-off-by: David O'Rourke <david.orourke@gmail.com>
@phyber phyber force-pushed the 20220831-add_family_remove_label_set branch from 79ff924 to d8c9bce Compare September 11, 2022 19:42
@phyber
Copy link
Contributor Author

phyber commented Sep 11, 2022

Rebased onto the current master and added entries to the CHANGELOG for this PR. I tried to use the same type of language in the CHANGELOG that was used the last time new methods were added to Family.

I didn’t bump the version in Cargo.toml since I guess this will all be released under the (currently unreleased) v0.19.0.

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.

🙏

@mxinden mxinden merged commit 1742d51 into prometheus:master Sep 15, 2022
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