Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macros: Handle constant field names #2617

Merged
merged 8 commits into from Sep 5, 2023
Merged

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Jun 15, 2023

Motivation

I've found myself in the case where I wanted to have customized event field name for different trait implementations. In fact, these implementations are completely unrelated (in separate applications), so, in this use case, I find more readable to have foo="some_id" and bar=16 instead of resource="foo" value="some_id" and resource=bar value=16

Because events only accept identifier or literal as field name, this is quite cumbersome/impossible to do. A simple solution could be to make events accept constant expression too; in my use case, I could then add a associated constant to my trait.

Solution

This PR proposes a new syntax for using constant field names:

tracing::debug!({ CONSTANT_EXPR } = "foo");

This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro expansion have been prefixed with __, e.g. __CALLSITE.

@wyfo wyfo requested review from hawkw, davidbarsky and a team as code owners June 15, 2023 16:23
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm definitely open to adding this. It would be nice to add tests for a few more cases, if you don't mind?

We may also want to allow constants to be used as span names and/or targets for spans and events, but we could look into this in a follow-up PR.

tracing/src/lib.rs Outdated Show resolved Hide resolved
.run_with_handle();
with_default(collector, || {
const FOO: &str = "foo";
tracing::event!(Level::INFO, { FOO } = "bar");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add a test which mixes constant and non-constant field names? i'd like to ensure that the macro parses unambiguously when the two forms are mixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may also want to test constant field names that are not in the curren scope, like { some_module::FOO } and { some_module::SomeType::FOO }?

.run_with_handle();
with_default(collector, || {
const FOO: &str = "foo";
tracing::span!(Level::TRACE, "my_span", { FOO } = "bar");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, let's also test some spans with mixed constant and ident field names

wyfo and others added 2 commits June 22, 2023 09:17
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@wyfo
Copy link
Contributor Author

wyfo commented Jun 22, 2023

Sorry for the delay, hard to find time for open-source at this moment...
I've updated the tests, it's indeed better now.

We may also want to allow constants to be used as span names and/or targets for spans and events, but we could look into this in a follow-up PR.

Actually, it's already possible, because they are all expr designators;

tracing::span!(std::convert::identity(tracing::Level::INFO), stringify!(my_span))

works fine for example.
On the other hand, an additional syntax was needed for fields because they use ident designator by default.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, thank you! i had one suggestion for making the documentation a little bit more obvious.

tracing/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +426 to +441
tracing::event!(
Level::INFO,
{ std::convert::identity(FOO) } = "bar",
{ "constant string" } = "also works",
foo.bar = "baz",
"quux"
);
tracing::event!(
Level::INFO,
{
{ std::convert::identity(FOO) } = "bar",
{ "constant string" } = "also works",
foo.bar = "baz",
},
"quux"
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for testing all these cases!

@hawkw hawkw enabled auto-merge (squash) September 5, 2023 21:15
@hawkw hawkw merged commit 245d607 into tokio-rs:master Sep 5, 2023
56 checks passed
@stormshield-frb
Copy link

Hi. Great useful feature, thanks !!

While testing it out, I've noticed that it seems to work only for the tracing::event! and tracing::span! macro. Is there any plan to integrate this feature with the #[instrument] macro and all the other 'classic' macros (debug, info, warn, etc) ? Thanks

davidbarsky pushed a commit that referenced this pull request Sep 26, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky pushed a commit that referenced this pull request Sep 29, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Oct 1, 2023
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <joseph.perez@numberly.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw pushed a commit that referenced this pull request Oct 13, 2023
# 0.1.39 (October 12, 2023)

This release adds several additional features to the `tracing` macros.
In addition, it updates the `tracing-core` dependency to
[v0.1.32][core-0.1.32] and the `tracing-attributes` dependency to
[v0.1.27][attrs-0.1.27].

### Added

- Allow constant field names in macros ([#2617])
- Allow setting event names in macros ([#2699])
- **core**: Allow `ValueSet`s of any length ([#2508])

### Changed

- `tracing-attributes`: updated to [0.1.27][attrs-0.1.27]
- `tracing-core`: updated to [0.1.32][core-0.1.32]
- **attributes**: Bump minimum version of proc-macro2 to 1.0.60
  ([#2732])
-  **attributes**: Generate less dead code for async block return type
   hint ([#2709])

### Fixed

- Use fully qualified names in macros for items exported from std
  prelude ([#2621], [#2757])
- **attributes**: Allow [`clippy::let_with_type_underscore`] in
  macro-generated code ([#2609])
- **attributes**: Allow `unknown_lints` in macro-generated code
  ([#2626])
- **attributes**: Fix a compilation error in `#[instrument]` when the
  `"log"` feature is enabled ([#2599])

### Documented

- Add `axum-insights` to relevant crates. ([#2713])
- Fix link to RAI pattern crate documentation ([#2612])
- Fix docs typos and warnings ([#2581])
- Add `clippy-tracing` to related crates ([#2628])
- Add `tracing-cloudwatch` to related crates ([#2667])
- Fix deadlink to `tracing-etw` repo ([#2602])

[#2617]: #2617
[#2699]: #2699
[#2508]: #2508
[#2621]: #2621
[#2713]: #2713
[#2581]: #2581
[#2628]: #2628
[#2667]: #2667
[#2602]: #2602
[#2626]: #2626
[#2757]: #2757
[#2732]: #2732
[#2709]: #2709
[#2599]: #2599
[`let_with_type_underscore`]: http://rust-lang.github.io/rust-clippy/rust-1.70.0/index.html#let_with_type_underscore
[attrs-0.1.27]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.27
[core-0.1.32]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants