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

[Feature] Adds a histogram label #2357

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

joske
Copy link
Contributor

@joske joske commented Feb 15, 2024

Motivation

This adds a histogram_label method to allow passing a label to the histogram. A limitation is that you can only attach one label.

@vicsn
Copy link
Contributor

vicsn commented Feb 15, 2024

@collinc97 can you adjust the settings so we can add @miazn as reviewer?

@miazn
Copy link

miazn commented Feb 15, 2024

We can actually removeregister_histogram function, as the histogram! macro (just like counter or gauge) registers the metric the first time it sees a new metric name (you can see this in lines 93 and 98)-- since we are using the base metrics definitions of the counter,gauge and hist.

@@ -98,3 +98,17 @@ pub fn histogram<V: Into<f64>>(name: &'static str, value: V) {
let histogram = ::metrics::histogram!(name);
histogram.record(value.into());
}

pub fn histogram_label<V: Into<f64>>(name: &'static str, label_key: &'static str, label_value: String, value: V) {
let labels = vec![::metrics::Label::new(label_key, label_value)];
Copy link

Choose a reason for hiding this comment

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

if we are only using a string label key and value, it might be easier to just use the macro, as i did here:
https://github.com/AleoHQ/snarkOS/pull/3064/files#diff-8663d09365b76641c32c45870cbd1915ab198258eb6b823b0aa779128a5b4ff1R93

this makes it cleaner to add the label function to the other two types (counter and gauge), which we should probably do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, now I switched to owned string for the label value, it's possible to use the macro. This was giving trouble with 'static when it was still a &str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add label variants to the gauge and counter too, but there's no need at the moment?

@joske
Copy link
Contributor Author

joske commented Feb 15, 2024

I kept the register functions for backwards compatibility. They were already useless when we bumped to metrics 0.22.

@joske joske force-pushed the feat/histogram_labels branch from ecf291e to 839175e Compare February 15, 2024 14:01
@howardwu howardwu merged commit b396a51 into AleoNet:mainnet Feb 15, 2024
78 checks passed
@howardwu howardwu changed the title feat: histogram label [Feature] Adds a histogram label Feb 16, 2024
@joske joske deleted the feat/histogram_labels branch June 25, 2024 07:13
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