Closed hlbarber closed 1 year ago
Posted this in a draft state to garner comments before polishing it up.
@hlbarber High-level thoughts: this is pretty straightforward, but the macros should probably look more like the tracing
macros.
In and of itself, Metadata
feels as straightforward as I imagined it to be, but the lack of macro examples and the copied approach of how the describe_*
macros handle detecting the unit parameter have solidified my thoughts around the macros needing to handle this better and in a way that's entirely compatible with the current macros. This is separate from actually providing level-prefixed variants of the macros, which I'm still a little hesitant about.
Essentially, what I'd want to see is:
target
and level
require a struct field notation in the macro arguments (like tracing
)target
or level
fields, or as labels (label parsing logic stays as-is)target
or level
after we've parsed something label-like (set a flag once we see the first non-target
/level
arg, basically)Or with examples:
register_counter!("metric_name");
register_counter!("metric_name", "label_a" => "...");
register_counter!("metric_name", target: "overridden");
register_counter!("metric_name", level: Level::TRACE, &some_vec_of_labels);
register_counter!("metric_name", target: "overridden", level: Level::Info, "service" => "foo");
Ultimately, seeing this PR, I'd say that you could keep going with everything else on your list but hold off on the info_counter!
stuff. If you're willing to take a crack at the procedural macro changes I've proposed above, that'd be good to. Otherwise, I would likely submit those as changes directly to your PR in the next week or two.
tracing::span
and tracing::event
, in order, accepts:
target: {target}
Level::{LEVEL}
"{name}"
(for span!
)This implementation, in order, accepts:
target: {target}
Level::{LEVEL}
(for backwards compatibility)"{name}"
register_counter!("metric_name");
register_counter!("metric_name", "label_a" => "...");
register_counter!(target: "overridden", "metric_name");
register_counter!(Level::TRACE, "metric_name", &some_vec_of_labels);
register_counter!(target: "overridden", Level::Info, "metric_name", "service" => "foo");
target and level require a struct field notation in the macro arguments
I can change level: Level::{LEVEL}
or make Level::{LEVEL}
required (like tracing
) - either of these will avoid the "unit parameter approach" (which I agree should perhaps be avoided). I think the struct field notation helps for optional fields.
Side note: Perhaps the unit parameter should be struct field notation too? Maybe this allows these macros to be done declaratively rather than procedurally.
metric name is always the first argument
Can do, but this seems like a departure tracing
where
macro_rules! span {
(target: $target:expr, parent: $parent:expr, $lvl:expr, $name:expr) => { ... };
(target: $target:expr, parent: $parent:expr, $lvl:expr, $name:expr, $($fields:tt)*) => { ... };
I can change
level: Level::{LEVEL}
or makeLevel::{LEVEL}
required (liketracing
) - either of these will avoid the "unit parameter approach" (which I agree should perhaps be avoided). I think the struct field notation helps for optional fields.
Right, I want level
to use the struct field notation because it it's optional.
Side note: Perhaps the unit parameter should be struct field notation too? Maybe this allows these macros to be done declaratively rather than procedurally.
The hard-coded bits of the describe_*!
macros are going away with the rework to metric "attributes" -- https://github.com/metrics-rs/metrics/issues/337#issuecomment-1511547332 -- so I would just ignore that for the moment.
Can do, but this seems like a departure
tracing
where
Fair point. I wasn't thinking we'd necessarily mirror everything about how the tracing
macros do it, but 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'm also not against doing that either -- proc macros are heavy -- but it might need to be staged as an improvement that comes after this and metrics attributes, because those two items get rid of the grossest parts of the procedural macros, which is the wishy-washy handling of specific positional arguments (i.e. is this arg a Unit
? let's check some hardcoded paths).
Let's stick with the current tracing
-inspired approach for now.
I'm going to try and take a look, in a few days, at just finalizing the remaining requested changes myself since it's so close.
"Rust / feature-check" is throwing a note: /usr/bin/ld: final link failed: No space left on device
, any suggestions?
Motivation
Closes https://github.com/metrics-rs/metrics/issues/370
Changes
&Metadata
argument to theRecorder::register_*
methodstarget: *
andLevel::*
arguments to macros.target
defaults tomodule_path!()
Level
defaults toLevel::INFO
.TODO
info_counter!
,warn_counter!
, etc