search

metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.11k stars 156 forks source link

Annotate metric types with must_use #475

Closed Dav1dde closed 5 months ago

Dav1dde commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Rebased and tests are passing locally.

tobz commented 5 months ago

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

tobz commented 4 months ago

Released in metrics@v0.23.0.

Thanks again for your contribution. 🙏🏻

rukai commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 :+1: