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

Annotate metric types with must_use #475

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

Dav1dde
Copy link
Contributor

@Dav1dde Dav1dde commented Apr 24, 2024

Happened to me just now, I created a counter and never incremented it and was wondering where my metric is going or if that piece of code was ever ran.

Annotate the metric types with must use and let the compiler warn us if they are never used.

@tobz
Copy link
Member

tobz commented Apr 26, 2024

I just pushed a fix to main for the stupid MSRV-related breakage in CI, so if you rebase to pick that up, I presume CI should go green.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Apr 26, 2024

Rebased and tests are passing locally.

@tobz tobz merged commit cd7de8c into metrics-rs:main Apr 26, 2024
12 checks passed
@tobz
Copy link
Member

tobz commented Apr 26, 2024

Woops, missed approving this... but I guess it doesn't really matter. 😂

@tobz tobz added C-core Component: core functionality such as traits, etc. E-simple Effort: simple. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. T-ergonomics Type: ergonomics. labels Apr 26, 2024
@Dav1dde Dav1dde deleted the must_use branch April 26, 2024 20:03
@tobz
Copy link
Member

tobz commented May 27, 2024

Released in metrics@v0.23.0.

Thanks again for your contribution. 🙏🏻

@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label May 27, 2024
@rukai
Copy link
Contributor

rukai commented Jun 3, 2024

I'm not sure if this was actually a good idea.
Creating a metric without using it isnt actually a noop.
Say if we call counter!("foo") without actually calling increment on it, it will still register a metric named "foo" without incrementing it. This can be useful for metrics for which passing around a Counter instance is not feasible but we still want to register the metric on startup so that when metrics are queried it shows up as 0, rather than not existing at all.

An argument could be made that calling counter! every time is terrible for performance, but my understanding is that if you pass in your name and labels as literals then it should be largely fine. I havent actually measured it though.

In fact for the usage in my project I think that all usages that trigger this warning should actually be fixed to not trigger the warning for performance reasons, but it still strikes me as a false positive.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jun 3, 2024

Then you can explicitly ignore the return value let _ = counter!(). Usually not using a counter is a bug, if you still want to do this for some reason you can opt out, it's similar on how Futures or Results in Rust work.

@rukai
Copy link
Contributor

rukai commented Jun 3, 2024

Not using a future is always a bug.
Ignoring a Result can be done with the .ok() method.
In this case we need to ignore sometimes but we also dont have an equivalent of the .ok() method.

But you are right, let _ = counter!() is a very easy workaround.
I think its better to warn people that there might be an issue and then let them explicitly ignore it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants