Support literals for field names. #790
Conversation
There was a problem hiding this comment.
I'm somewhat on the fence about introducing the ability to create fields whose names are arbitrary strings in the macros. We currently restrict them to just Rust identifiers or dotted sequences of Rust identifiers to limit the complexity handled by Subscriber implementations and create conventions for things like field name namespacing that are usable by all subscribers. In an ideal world, I think we would prefer to generate field names in the format that OpenTracing prefers by using the conventional . character for namespacing, so that the namespacing convention is also consumable by other subscribers more easily.
However, your point about the additional complexity for handling this is reasonable, especially if the . character is also meaningful to OpenTracing. Also, this makes it easier to use Rust reserved words (such as, for example, type) as field names without requiring subscribers to special case a version of the "raw identifier" r#type syntax.
Hopefully users in libraries that don't know what subscriber will consume their traces will continue to use dotted identifier sequences as field names wherever possible, and the new syntax for less restricted field names will be used only where necessary.
I'd love to hear if @davidbarsky or @LucioFranco have an opinion as well.
Setting all of that aside, I noticed a couple issues with the implementation and documentation that it would be good to fix up before making this change.
There was a problem hiding this comment.
I had a small suggestions for the examples. Beyond that, this looks good to me!
Also, it's probably worth adding a couple cases with literal field names to the macro compilation tests here: https://github.com/tokio-rs/tracing/blob/master/tracing/tests/macros.rs in order to guard against regressions breaking this syntax.
| @@ -262,6 +262,22 @@ fn error_span_with_parent() { | |||
| error_span!(parent: &p, "bar",); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn span_with_non_rust_symbol() { | |||
There was a problem hiding this comment.
May be good to add a similar compile test for an event, just in case. This is probably sufficient, since a regression would probably be in the shared key-value parsing macros, so it may not matter a whole lot...
There was a problem hiding this comment.
We only change valueset and fieldset, not span or event directly, so I think just testing span! is fine, unless things get rearranged radically.
This permits : in field names but also potentially anything stringifiable, which may be overly liberal. Signed-off-by: Robert Collins <[email protected]>
done |
|
And merged! Thanks for working on this! |
|
You're welcome. Thank you for merging it. :) |
Fixed - Fixed a bug where `LevelFilter::OFF` (and thus also the `static_max_level_off` feature flag) would enable *all* traces, rather than *none* (#853) - **log**: Fixed `tracing` macros and `Span`s not checking `log::max_level` before emitting `log` records (#870) Changed - **macros**: Macros now check the global max level (`LevelFilter::current`) before the per-callsite cache when determining if a span or event is enabled. This significantly improves performance in some use cases (#853) - **macros**: Simplified the code generated by macro expansion significantly, which may improve compile times and/or `rustc` optimizatation of surrounding code (#869, #869) - **macros**: Macros now check the static max level before checking any runtime filtering, improving performance when a span or event is disabled by a `static_max_level_XXX` feature flag (#868) - `LevelFilter` is now a re-export of the `tracing_core::LevelFilter` type, it can now be used interchangably with the versions in `tracing-core` and `tracing-subscriber` (#853) - Significant performance improvements when comparing `LevelFilter`s and `Level`s (#853) - Updated the minimum `tracing-core` dependency to 0.1.12 (#853) Added - **macros**: Quoted string literals may now be used as field names, to allow fields whose names are not valid Rust identifiers (#790) - **docs**: Several documentation improvements (#850, #857, #841) - `LevelFilter::current()` function, which returns the highest level that any subscriber will enable (#853) - `Subscriber::max_level_hint` optional trait method, for setting the value returned by `LevelFilter::current()` (#853) Thanks to new contributors @cuviper, @ethanboxx, @ben0x539, @dignati, @colelawrence, and @rbtcollins for helping out with this release! Signed-off-by: Eliza Weisman <[email protected]>
### Fixed - Fixed a bug where `LevelFilter::OFF` (and thus also the `static_max_level_off` feature flag) would enable *all* traces, rather than *none* (#853) - **log**: Fixed `tracing` macros and `Span`s not checking `log::max_level` before emitting `log` records (#870) ### Changed - **macros**: Macros now check the global max level (`LevelFilter::current`) before the per-callsite cache when determining if a span or event is enabled. This significantly improves performance in some use cases (#853) - **macros**: Simplified the code generated by macro expansion significantly, which may improve compile times and/or `rustc` optimizatation of surrounding code (#869, #869) - **macros**: Macros now check the static max level before checking any runtime filtering, improving performance when a span or event is disabled by a `static_max_level_XXX` feature flag (#868) - `LevelFilter` is now a re-export of the `tracing_core::LevelFilter` type, it can now be used interchangably with the versions in `tracing-core` and `tracing-subscriber` (#853) - Significant performance improvements when comparing `LevelFilter`s and `Level`s (#853) - Updated the minimum `tracing-core` dependency to 0.1.12 (#853) ### Added - **macros**: Quoted string literals may now be used as field names, to allow fields whose names are not valid Rust identifiers (#790) - **docs**: Several documentation improvements (#850, #857, #841) - `LevelFilter::current()` function, which returns the highest level that any subscriber will enable (#853) - `Subscriber::max_level_hint` optional trait method, for setting the value returned by `LevelFilter::current()` (#853) Thanks to new contributors @cuviper, @ethanboxx, @ben0x539, @dignati, @colelawrence, and @rbtcollins for helping out with this release! Signed-off-by: Eliza Weisman <[email protected]>
This permits : in field names but also potentially anything
stringifiable, which may be overly liberal.
Signed-off-by: Robert Collins [email protected]
Motivation
Some opentracing systems use fields with : in the names, which are not legal rust identifiers. We could special case :, but then we have complicated double-nested patterns (repeated idents separated by : and a nested repeat separated by .), and this may by a case of always being one-step-behind as more cases turn up; so this patch instead just gets in front and lets users put in whatever they want: as they are not rust identifiers, they miss out on some niceness.
Solution