migrate from procedural to declarative macros

[hlbarber]

Changes

• Remove metrics-macros crate
• Rewrite all existing macros using macro_rules within the metrics crate

The goal here is to not break users and to also maintain the "fast paths" taken by the procedural macro where literals cause static to be used.

This PR is intended as a place of discussion, once it is sufficiently mature I'll close it and reopen a polished PR.

[hlbarber]

I've picked register_counter to migrate first - from the initial experimentation it feels possible. We're passing cargo t --no-fail-fast with a 2 exceptions.

[tobz]

So far, so good... and in retrospect, much simpler than procedural macros. 😂

Left some feedback.

[tobz]

I didn't even realize the curly braces form for calling macros is a thing (TIL!) but for the sake of consistency with most code in the wild, I'd prefer us to do the normal macro_name!(...) form across the board.

[tobz]

Overall, this is seeming pretty reasonable and straightforward.

It does look like some tests are broken, though, specifically around how we're handling building the key, and the expectation that we can always borrow it at the callsite, even when it may already be borrowed.

[hlbarber]

The following seem to fix the failing test cases while maintaining the fast paths where possible. Please tell me if this looks off in any way.

https://github.com/metrics-rs/metrics/pull/386/files/c356678f3bd0bab76ad6932a951374ca5a1e484d..5e82f3e41e5ad2034f03359286d2e1a01eb775cf

[hlbarber]

That's the "counter" cross section of macros replaced. After register_counter it isn't really involved.

Given near perfect symmetry between the metric types I don't foresee any nasty obstructions when transitioning the other macros.

I'll run benchmarks shortly to check for any regressions.

[hlbarber]

After restoring doc tests there's a bit more to do, will look into it tomorrow.

[hlbarber]

0a32d83 has made is considerably more branch-y, but still tractable imo. Rather than making use of ? we must enumerate the combinations.

I think with some tinkering this can be compacted. I think perhaps we can parse the arguments as one (or two?) tt and then pass them onto other branches - some prior art can be found in the tracing macros (info etc).

I can push for compactness now, but given #369 is around the corner I can see an argument for postponing.

[tobz]

I can push for compactness now, but given #369 is around the corner I can see an argument for postponing.

I'd say let's just get the logic correct now and deal with condensing later.

I think with some tinkering this can be compacted. I think perhaps we can parse the arguments as one (or two?) tt and then pass them onto other branches - some prior art can be found in the tracing macros (info etc).

I'm not against the verbose-ness of having a branch for each possible permutation... so long as there's a viable path to, in the future, generalizing that macro logic across the different metric types. Basically, it'd be a small tragedy to just have to duplicate all of it over and over again when we do gauges and histograms... just ripe for introducing a subtle difference.

Having one register_metric macro or something, for example, that we just call into and pass the metric type-appropriate idents, and so on... and it has all of the various branches? That'd be fine.

[hlbarber]

Having one register_metric macro or something, for example, that we just call into and pass the metric type-appropriate idents, and so on... and it has all of the various branches? That'd be fine.

A reason I haven't re-used register_counter in the current impl is that the branch, in metric_macros::get_register_and_op_code for register_counter (found here) uses metrics::recorder and the counter/absolute_counter/etc branch uses metrics::try_recorder (found here).

If I use metrics::recorder for the counter branch I get a regression on the "uninitialized" benchmarks - this is a little suprising to me, I'd expect the NoopRecorder, after optimizations, to be equal in performance to checking Some(_) = try_recorder. I wonder if this is still the case if we implemented Recorder for Option<T> where T: Recorder and made recorder return it, rather than dyn Recorder.

[tobz]

I'd say switch to try_recorder across the board.

The regression is surprising to me too, but also, thinking about it... I'm not sure I actually give a crap if it's like 5ns vs 20ns or whatever when a recorder isn't installed. If it ever matters enough to me, or someone else, well... we can figure something out then.

[hlbarber]

Rest of the macros are migrated. I've DRY'd it up a little bit with the describe and method macros. I haven't taken the try_recorder change yet because it deduplicated less with method macro in place.

I played around a little bit with $(args: tt),*, trying something like

macro_rules counter {
    ($($args:tt),) => $crate::method(register_counter, increment, $($args),*)
}

similar with what we see in the tracing macros. This doesn't work because of the mix of delimiters we see in the macros (, and =>). You can see the exact problem here https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=def715a3d053a4b933ccb4a821260729. Perhaps this is something the next API will side step. I think there's also an argument to keeping the explicit branches in the "macro signatures" to serve as documentation.

metrics-macro is now empty. I'm guessing I should delete the crate from the workspace entirely so that we don't release an empty crate?

[tobz]

Perhaps this is something the next API will side step. I think there's also an argument to keeping the explicit branches in the "macro signatures" to serve as documentation.

I think things are fine as is. Good to see your example as it makes it clear, but in seeing it and seeing what you chose to do instead, it makes me think that making it perfectly DRY isn't as big a readability win as I thought it might be anyways.

metrics-macros is now empty. I'm guessing I should delete the crate from the workspace entirely so that we don't release an empty crate?

Yessss, nuke it from orbit. 👨🏻‍🚀

[tobz]

Hmm, why doesn't this one need to be fully qualified?

[hlbarber]

It does, whoops. I'll try write a test involving #![no_implicit_prelude] so that me and others wont make this mistake again.

[hlbarber]

a901339

I've slipped #![no_implicit_prelude] into the doc tests.

I think it might be better to write a dedicate test (or even just an example) where we invoke all combinations of all macros with #![no_implicit_prelude] and we don't import things like use std::convert::From which weakens the test. I can do this in a follow up PR unless you think its important now.

[tobz]

Sounds like it can be a follow-up, to be honest.

[tobz]

I plan on taking a final look at this over the next few days.

[hlbarber]

I can transplant this into a formal PR with a proper description now if you'd like?

[tobz]

Nah, no worries. At most, feel free to update the PR title and description and mark it as ready to review and all of that... but no further cleanup/tidying required.

[tobz]

Overall, this looks really good and most of the comments are nits. There's one comment about a broken rustdoc link that needs to be fixed, however.

[hlbarber]

On my machine cargo bench, on main vs this branch, shows no obvious regressions - worth double checking on your machine though.

[tobz]

Overall, this looks good to me.

Thank you for working to get this to the finish line. ❤️

[tobz]

Released as metrics@v0.22.0.

Thanks again for your contribution. ❤️