metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.08k stars 148 forks source link

remove macros, retaining only `register_*` and `describe_*` macros #394

Closed hlbarber closed 10 months ago

hlbarber commented 11 months ago

Motivation

Simplification of the existing macro API

https://github.com/metrics-rs/metrics/issues/369

Changes

LukeMathWalker commented 10 months ago

What prevents the describe_* macro family from being converted into a set of methods on the respective metric types? Having a single macro per metric type would be quite interesting—it would entirely remove ambiguity when it comes to "what macro do I need".

hlbarber commented 10 months ago

What prevents the describe_* macro family from being converted into a set of methods on the respective metric types?

Yeah removing the family of describe_* macros and adding them as just an extra methods on the handles would be super slick.

The obstruction I see here is that Recorder::describe_* takes a KeyName (example) whereas Recorder::register_*takes Key (example), meaning that register_* requires labels to be specified whereas describe_* doesn't. Given that the current counter! API accepts label arguments it might be confusing that counter!("name", a => b).describe(...) disregards a => b.

What you could imagine is moving the label arguments from counter! and putting them into the increment method - counter!("name").increment(3, labels) or maybe counter!("name").labels(labels).increment(3). This would mean that we forgo some of the performance conscious "specialization" taken by the macro - would have to benchmark to check for regressions.

tobz commented 10 months ago

I have Thoughts (tm) about all of this, and it's probably worth a new issue to avoid polluting this PR.

tobz commented 10 months ago

Just gonna merge since the feature checks are known to go boom on the free GHA runners... something I need to eventually take care of.

kornelski commented 3 months ago

This change wasn't mentioned in the changelog.

tobz commented 3 months ago

Ah yeah, it does look like I forgot to add a link to RELEASES.md in the change log for this specific change.

Easy enough to backport. Thanks for reporting. đź‘Ť