Evaluate whether or not counter-related macros can be renamed/simplified.

[tobz]

As mentioned in #338, it can be confusing when trying to figure out which counter macro to use for a given scenario, as we have counter!, increment_counter!, and absolute_counter!.

We should consider if there's any potential simplification we could apply. One form this proposed simplification could take is around exploring if one macro could serve both the "increment by N" and "increment by 1" use cases, rather than having them be distinct macros.

[tobz]

/cc @hlbarber @LukeMathWalker

[hlbarber]

Here's a working proposal to get discussion started. Please "Tools -> Expand macros" and inspect.

We make use of $args:tt tricks here to dramatically reduce the footprint of the macro declaration. For brevity we omit the trivial aspects of the macros which are demonstrated in https://github.com/metrics-rs/metrics/pull/386. For example, short cutting is omitted because it amounts to an extra branch with a literal type.

The proposal I'm making is that we replace counter!, increment_counter!, absolute_counter!, register_counter! (and others such as decrement_gauge! with the following, while leaving describe_counter the same.

fn main() {
    // Comments are removed during macro expansion so we're just delimiting with `struct` here
    counter!("foo", +, 1);
    counter!("bar", =, 3);
    counter!(level: Level::DEBUG, "baz", +, 1);
    counter!(target: "custom target", "baz", =, 1);
    counter!(target: "custom target", level: Level::TRACE, "baz", =, 1);
    counter!("foo", +, 1, ("bar1", "baz1"), ("bar2", "baz2"));
    counter!("foo", +, 1, "bar", "foo");
    let counter: Arc<dyn CounterFn> = counter!("foo");
    let counter: Arc<dyn CounterFn> = counter!("foo", ("bar", "baz"));
}

Any assertion below should be prefixed with "as far as I can tell".

An API such as

counter!("name" += 1);

counter!("name" = 1);

is not possible because expr, which is required for dynamic names, cannot be followed by an operator (ident and literal can which is why tracing can do $field_name = $field_value).

Having label key/value pairs grouped into a tuple means we can use args:tt more effectively here - it means that $args:tt will match the entire ($key, $value) rather than $key then $value.

[hlbarber]

I think it's important that others post APIs they'd like here and we can compare the merits.

I'd like to set up a boundary between the feasible and unfeasible APIs. If you are unable to get a Rust playground proof-of-concept working I think it's still valuable to post and describe why you found it difficult. Maybe someone else will be able to make it work.

[hlbarber]

Here's a working proposal to get discussion started. Please "Tools -> Expand macros" and inspect.

One of the problems with this design, imo, is that counter!("name", "key_a", "key_b") is a bit confusing. Perhaps labels should be declared in the form labels: $expr, labels: [$($pairs),*], and labels: [$(($key, $value)),*]?

counter!("name", +, 1, labels: ["key_a", "key_b"])

or even

counter!(labels: ["key_a", "key_b"], "name", +, 1)

[hlbarber]

Another route here is that we simply don't have any macro arguments for +, -, or = etc. Instead we just have

counter!("name").set(value);
counter!("name").add(value);

The more I think about it the more I'm in favor of this approach. If we go down this path perhaps we should make the *Fn APIs more consistent - add, subtract, set being the three methods here.

[tobz]

At first blush, I viscerally dislike the variant with the operation sigil (+, -, etc) as one of the macro arguments. 😂 An interesting thought, but too clever by half in practice, I'd imagine.

That said, the overarching theme of "can we use one macro to do more than one operation?" is a good theme. The idea around pushing towards directly calling methods for the relevant operation (i.e. counter!(...).add(n)) seems like the simplest to implement and to grok as a user. It's also closer, in practice, to most other APIs for crates that provide metrics handling, which wouldn't hurt.

[tobz]

Just talking out loud.... the pros/cons for the "use methods instead of individual operation macros":

Pros

• Fewer macros overall!
• We can keep the metric name/target/level/labels handling fairly close to what it currently is: only the value for the operation would be moved as an argument to the operation method call.
• IDE support for documentation around actual methods/functions has always been vastly better IME than for macros, which would be good because I feel like documenting the nuances of the API, macros, etc, has always been a challenge.
• Gives us a way to easily support your idea above of the macro being able to serve both as a registration macro (let foo = counter!(...)) as well as an emission macro (counter!(...).add(1)) without having to thread the needle in terms of macro branch argument specificity.

Cons

• Breaking change. We are pre-1.0, but still a breaking change nonetheless.
• Somewhat unconventional, as the general pattern for macros is that they do everything within the call, or they're just used as a way to generate an expression, rather than ever used in a chained fashion. [1]

1. I admit that this is more of a nit than anything else, but a lot of my design mentality strives to think about how we can do things that line up with common patterns/idioms so that at least writing the code feels "normal", and that we aren't necessarily (ab)using macros to depend on weird/complex syntax to get the job done, etc.

[hlbarber]

Here's the proof-of-concept using just methods on Counter.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3d0289dd31227cd1dab26bd6087b6748

A nicety that could become standard practice if we adopted the "use methods" approach:

If

impl AddAssign<u64> for Counter {
    fn add_assign(&mut self, value: u64) {
        self.increment(value);
    }
}

we can

counter!("name") += 1;

Similarly, for SubtractAssign. It's a shame there's no trait backing = else we could have counter!("name") = 3 too.

[LukeMathWalker]

IDE support for documentation around actual methods/functions has always been vastly better IME than for macros, which would be good because I feel like documenting the nuances of the API, macros, etc, has always been a challenge.

This is a massive pro in my opinion—discovering what a macro can and cannot do is an exercise in careful examination of the docs, while methods combine super-nicely with in-editor auto-completion. They are also very searchable in the auto-generated API reference.

[LukeMathWalker]

Gives us a way to easily support your idea above of the macro being able to serve both as a registration macro (let foo = counter!(...)) as well as an emission macro (counter!(...).add(1)) without having to thread the needle in terms of macro branch argument specificity.

Somewhat unconventional, as the general pattern for macros is that they do everything within the call, or they're just used as a way to generate an expression, rather than ever used in a chained fashion. [1]

An idea on this topic: if the macro returned a type that was marked as #[must_use] you can further nudge the user towards the mindset of "I need to do something with it", which would further mitigate the (minor) strangeness of the API.
I wonder if registration itself should be a method call.

[hlbarber]

I agree that #[must_use] is definitely something to be considered here and requiring counter!("name").register() would remove the case when we don't want to use.

I'm guessing the implementation looks something like this?

#[must_use]
struct CounterGuard<'a> { recorder: &'a Recorder, metadata: &'a Metadata, key: &'a Key }

impl CounterGaurd {
    fn register(&self)  -> Counter { self.recorder.counter(self.metadata, self.key) }

    fn add(&self, value: u64) { self.register().add(value) }
}

let guard: CounterGuard = counter!("name");

[hlbarber]

I'm happy to draft out a PR for this when a direction is decided on https://github.com/metrics-rs/metrics/issues/369#issuecomment-1720970000. I think this makes sense:

counter!(target: "target", level: Level::DEBUG, labels: [foo, bar, baz], "name")
counter!(target: "target", level: Level::DEBUG, labels: [(foo_key, foo_value), (bar_key, bar_value)], "name")

[tobz]

I think my biggest question is: why the switch from <key> => <value> to tuples for labels?

[hlbarber]

I think my biggest question is: why the switch from => to tuples for labels?

Not a particularly strong argument and just writing this out has kinda convinced me that the existing label syntax is fine but: at the moment we accept

let dyn_vals = [(dyn_key_a, dyn_val_a), (dyn_key_b, dyn_key_b)];
counter!(&dyn_vals);
counter!(dyn_key_a => dyn_val_a, dyn_key_b => dyn_val_b);
counter!(dyn_pair_a, dyn_pair_b);
counter!("static_key_a" => "static_val_a", "static_key_b" => "static_val_b");

it feels a little more consistent if

let dyn_vals = [(dyn_key_a, dyn_val_a), (dyn_key_b, dyn_key_b)];
counter!(&dyn_vals);
counter!((dyn_key_a, dyn_val_a), (dyn_key_b, dyn_val_b));
counter!(dyn_pair_a, dyn_pair_b);
counter!(("static_key_a", "static_val_a"), ("static_key_b", "static_val_b"));

It does look like it adds some noise though.

Another alternative to => is =, like tracing.

counter!(dyn_key_a = dyn_val_a, dyn_key_b = dyn_val_b);
counter!(dyn_pair_a, dyn_pair_b);
counter!("static_key_a" = "static_val_a", "static_key_b" = "static_val_b");

or even :

counter!(dyn_key_a: dyn_val_a, dyn_key_b: dyn_val_b);
counter!(dyn_pair_a, dyn_pair_b);
counter!("static_key_a": "static_val_a", "static_key_b": "static_val_b");

Both a = b and (a, b) have the advantage that they can be parsed by expr, stmt whereas a => b and a: b cannot, this feels like it helps simplify the macro implementation but I'd have to spend more time experimenting to be sure.

I guess = makes more sense for tracing because all the field keys are idents rather than more general exprs.

https://github.com/metrics-rs/metrics/pull/380#issuecomment-1581238951

I suppose all named optionals having to come first would make parsing easier, especially if we attempted to rewrite the macros to be declarative.

I applied this to labels too, but on further consideration, if labels are going to be the last of the arguments then it doesn't really help using labels: from a parsing perspective.

[tobz]

Yeah, I realize that there's an asymmetry to what is technically accepted due to allowing both the key/value notation and expressions... but in my mind the difference is that the fact tuples are accept is a consequence of allowing expressions, not the macros inherently destructuring the actual tuple notation.

My thought is that if it's not much harder/complex to keep =>, and it doesn't somehow pigeonhole what can be done with the macros... then we should keep it, as it's one less breaking change.

[hlbarber]

I think we agree enough now and we can argue about the details in the PR. Will get it written up soon.

[hlbarber]

Are we happy with the state we've landed in now that https://github.com/metrics-rs/metrics/pull/394 is merged?

[tobz]

I would say that, yeah, this issue has been resolved with the merging of #394. 👍🏻