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.

Sign up for GitHub

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

Jump to bottom

string literal field names don't compile for level! event macros #2748

Closed
hawkw opened this issue Oct 12, 2023 · 0 comments · Fixed by #2883
Closed

string literal field names don't compile for level! event macros #2748

hawkw opened this issue Oct 12, 2023 · 0 comments · Fixed by #2883
Labels
crate/tracing Related to the `tracing` crate kind/bug Something isn't working

Comments

@hawkw
Copy link
Member

hawkw commented Oct 12, 2023

Bug Report

Version

tracing 0.1.37

Platform

Any

Crates

tracing

Description

Tracing's macros support string literals as field names. In the documentation, we give an example with a span:

Fields with names that are not Rust identifiers, or with names that are Rust reserved words,
may be created using quoted string literals. However, this may not be used with the local
variable shorthand.

// records an event with fields whose names are not Rust identifiers
//  - "guid:x-request-id", containing a `:`, with the value "abcdef"
//  - "type", which is a reserved word, with the value "request"
span!(Level::TRACE, "api", "guid:x-request-id" = "abcdef", "type" = "request");

This form works when used with the span! and event! macros. However, when used with the level event macros (e.g. info!, debug!, and friends), the string literal form doesn't compile --- the macro appears to interpret this as format arguments, rather than as fields. For example:

use tracing::{self, Level}; // 0.1.37

fn main() {
    let foo = "lol";
    // this compiles
    tracing::span!(Level::TRACE, "api", "guid:x-request-id" = ?foo);
    
    // this compiles
    tracing::trace_span!("api", "guid:x-request-id" = ?foo);
    
    // this compiles
    tracing::event!(Level::TRACE, "guid:x-request-id" = ?foo);
    
    // this doesn't compile
    tracing::trace!("guid:x-request-id" = ?foo)
}

(playground)

We should fix this.

Also, it appears that the macros.rs integration test doesn't contain any tests for this form: https://github.com/tokio-rs/tracing/blob/99e0377a6c48dd88b06ed2ae0259c62d8312c58d/tracing/tests/macros.rs

This is probably why it only works in some cases. 🙃
@hawkw hawkw added kind/bug Something isn't working crate/tracing Related to the `tracing` crate labels Oct 12, 2023
@mladedav mladedav mentioned this issue Feb 18, 2024
Merged
hawkw pushed a commit that referenced this issue Mar 1, 2024
@mladedav
tracing: fix event macros with constant field names in the first posi…
908cc43 
…tion (#2883)


## Motivation

Const argumetns in `level!` macros do not work when in the first
position.

This also seems to have fixed #2748 where literals for fields names like
`info!("foo" = 2)` could not be used outside the `event!` macro.


Fixes #2837
Fixes #2738

## Solution

Previsously, `level!($(args:tt))` was forwarded to `event!(target: ...,
level: ..., { $(args:tt) })` but the added curly braces seem to have
prevented the `event` macro from correctly understanding the arguments
and it tried to pass them to `format!`.

With this change there may have some performance impact when expanding
the macros in most cases where the braces could have been added as it
will take one more step.

These are the two relevant `event!` blocks I believe, the new tests used
to expand to the first one (with empty fields), now they expand to the
latter:
```
    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );
```
dpc pushed a commit to dpc/tracing that referenced this issue Apr 20, 2024
@mladedav @dpc
tracing: fix event macros with constant field names in the first posi…
cc02d08 
…tion (tokio-rs#2883)


## Motivation

Const argumetns in `level!` macros do not work when in the first
position.

This also seems to have fixed tokio-rs#2748 where literals for fields names like
`info!("foo" = 2)` could not be used outside the `event!` macro.


Fixes tokio-rs#2837
Fixes tokio-rs#2738

## Solution

Previsously, `level!($(args:tt))` was forwarded to `event!(target: ...,
level: ..., { $(args:tt) })` but the added curly braces seem to have
prevented the `event` macro from correctly understanding the arguments
and it tried to pass them to `format!`.

With this change there may have some performance impact when expanding
the macros in most cases where the braces could have been added as it
will take one more step.

These are the two relevant `event!` blocks I believe, the new tests used
to expand to the first one (with empty fields), now they expand to the
latter:
```
    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );
```
hds pushed a commit that referenced this issue Nov 21, 2024
@mladedav @hds
tracing: fix event macros with constant field names in the first posi…
7bb5bb2 
…tion (#2883)


## Motivation

Const argumetns in `level!` macros do not work when in the first
position.

This also seems to have fixed #2748 where literals for fields names like
`info!("foo" = 2)` could not be used outside the `event!` macro.


Fixes #2837
Fixes #2738

## Solution

Previsously, `level!($(args:tt))` was forwarded to `event!(target: ...,
level: ..., { $(args:tt) })` but the added curly braces seem to have
prevented the `event` macro from correctly understanding the arguments
and it tried to pass them to `format!`.

With this change there may have some performance impact when expanding
the macros in most cases where the braces could have been added as it
will take one more step.

These are the two relevant `event!` blocks I believe, the new tests used
to expand to the first one (with empty fields), now they expand to the
latter:
```
    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );
```
hds pushed a commit that referenced this issue Nov 22, 2024
@mladedav @hds
tracing: fix event macros with constant field names in the first posi…
2d7a770 
…tion (#2883)


## Motivation

Const argumetns in `level!` macros do not work when in the first
position.

This also seems to have fixed #2748 where literals for fields names like
`info!("foo" = 2)` could not be used outside the `event!` macro.


Fixes #2837
Fixes #2738

## Solution

Previsously, `level!($(args:tt))` was forwarded to `event!(target: ...,
level: ..., { $(args:tt) })` but the added curly braces seem to have
prevented the `event` macro from correctly understanding the arguments
and it tried to pass them to `format!`.

With this change there may have some performance impact when expanding
the macros in most cases where the braces could have been added as it
will take one more step.

These are the two relevant `event!` blocks I believe, the new tests used
to expand to the first one (with empty fields), now they expand to the
latter:
```
    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crate/tracing Related to the `tracing` crate kind/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: level! macros with constant field names in the first position mladedav/tracing
1 participant
@hawkw