attributes: support arbitrary field expressions #672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added
kind/feature
|
I think the docs also need to be updated. Will fix that. |
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky
approved these changes
May 11, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
|
Okay, merging #711 into this was a bit of a mess, but I've got all the tests passing again so it should be good. @davidbarsky, do you want to give it another look? I did change a few things while merging. |
|
I’ll take a look. |
davidbarsky
approved these changes
May 23, 2020
This was referenced Jun 29, 2020
This was referenced Jul 7, 2020
hawkw
added a commit
that referenced
this pull request
Jul 8, 2020
## Motivation In PR #672, the `#[instrument]` attribute was rewritten to use a custom `syn` parser rather than using `syn::AttributeArgs`. This was primarily to allow the use of arbitrary expressions as fields, but it had the side benefit of also allowing more sophisticated error reporting. In particular, we were able to replace the previous behavior of silently ignoring unknown input with a compiler error, which has been requested a few times (see #772 and #727). However, emitting a compiler error for inputs that we previously allowed (even if they did nothing) is technically a breaking change --- see #773 (comment) and following comments. This means that the other new features added in #672 cannot be released until we release `tracing-subscriber` 0.2. ## Solution This branch avoids the breaking change by downgrading these errors to warnings. However, it isn't currently possible to emit a warning from a proc-macro on stable --- the `proc_macro::Diagnostic` API which would allow this requires a nightly feature. This branch hacks around this by generating a fake deprecated item with for each unrecognized token in the input, emitting the skipped item's parse error as the deprecation's `note`, and "using" the deprecated item. The deprecated warning is used here rather than other code that will emit compiler warnings, because it gives us a way to customize the warning output (via the `note` field). I've tried to make it fairly obvious that this is not a "real" deprecation. Sample output looks like this:  Fixes #727 Fixes #772 Signed-off-by: Eliza Weisman <eliza@elizas.website>
hawkw
added a commit
that referenced
this pull request
Jul 8, 2020
Added - **attributes**: Support for arbitrary expressions as fields in `#[instrument]` (#672) - **attributes**: `#[instrument]` now emits a compiler warning when ignoring unrecognized input (#672, #786) - Improved documentation on using `tracing` in async code (#769) Changed - Updated `tracing-core` dependency to 0.1.11 Fixed - **macros**: Excessive monomorphization in macros, which could lead to longer compilation times (#787) - **log**: Compiler warnings in macros when `log` or `log-always` features are enabled (#753) - Compiler error when `tracing-core/std` feature is enabled but `tracing/std` is not (#760) Thanks to @nagisa for contributing to this release!
hawkw
added a commit
that referenced
this pull request
Jul 8, 2020
### Added - **attributes**: Support for arbitrary expressions as fields in `#[instrument]` (#672) - **attributes**: `#[instrument]` now emits a compiler warning when ignoring unrecognized input (#672, #786) - Improved documentation on using `tracing` in async code (#769) ### Changed - Updated `tracing-core` dependency to 0.1.11 ### Fixed - **macros**: Excessive monomorphization in macros, which could lead to longer compilation times (#787) - **log**: Compiler warnings in macros when `log` or `log-always` features are enabled (#753) - Compiler error when `tracing-core/std` feature is enabled but `tracing/std` is not (#760) Thanks to @nagisa, and everyone who contributed to the new `tracing-core` and `tracing-attributes` versions, for contributing to this release!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Fields on spans generated by
#[instrument]consist of the parametersto the
instrumented function, and a list of optional fields withliteral values passed in the
fieldsargument of the attribute. Thecurrent API for additional user-defined fields is pretty limited, since
it only accepts literals (and doesn't accept dotted field names). It
would be nice to have an API for including additional fields with values
set by calling methods on
selfor on parameters, or by accessingvalues from parameters.
Solution
This branch extends the currently supported
fieldsargument to allowproviding arbitrary expressions which are invoked within the function's
scope. This is a superset of the previous support for literal values
only. In addition, field names passed to
fieldsmay now contain dottedRust identifiers.
Implementing this required rewriting how arguments to the
#[instrument]attribute are parsed. Previously, we usedsyn'sAttributeArgstype, which parses a comma-separated of "nested meta"items, consisting of
$ident($nested meta),$ident = $nested_meta,paths, and literals. By replacing the use of
AttributeArgswith ourown types implementing
syn::parse::Parse, we can accept moreexpressions in the attribute arguments position. This also lets us
reject more invalid inputs at parse-time, which improves syntax error
reporting a bit.
One thing that's worth noting is that the current implementation will
generate an empty field when a field name is provided without an
=and a value. This makes sense when only simple fields with literal
values are permitted. However, if we accept arbitrary expressions in the
macro, this is not the ideal behavior --- we would prefer to use the
same local variable shorthand as the function-like
tracingmacros.However, changing this now is a breaking change. Any code which uses a
name that doesn't exist in the current scope to declare an empty field
would fail to compile, because it attempts to reference a name that
doesn't exist. Instead, I left a comment noting that this is not the
ideal behavior and it should be changed next time we're ready to break
the proc macros.
Fixes: #650
Signed-off-by: Eliza Weisman eliza@buoyant.io