tracing icon indicating copy to clipboard operation
tracing copied to clipboard

Allow string literal field identifiers

Open valkum opened this issue 1 year ago • 8 comments

Fixes #2438

Motivation

Currently, you can only use Idents delimited by . for fields. This prevents us creating an empty field containing rust keywords such as type. (e.g. graphql.operation.type) The span! and event! macros allow the usage of string literals to circumvent this limitation.

tracing::event!(Level::TRACE, "field.type" = "test", "event name");

Solution

To allow for a similar API as with the macros, this PR replaces the Field.name with an enum FieldName. The enum can be the current Punctuated type or a LitStr.

Possible other solution

Another option would be to unraw the identifiers when emitting the field name. But this might cause confusion when an emitted field name does not match a struct field name in code.

I am open to providing a PR targeting the master branch as well.

valkum avatar Jun 01 '23 19:06 valkum

hey @valkum thanks for the code 🍻 I'm really interested in getting this into the crate, because it solves the issue of key names with type. Do you still want to work on it, otherwise I would create a new PR with your changes.

@hawkw as one of the tokio members, how do you feel about the code changes in the general?

gruebel avatar Jan 30 '24 07:01 gruebel

I can see if I can rebase the current PR in the coming days. Do you want two PRs one for 0.1.x and one for master?

valkum avatar Jan 31 '24 14:01 valkum

@valkum @hawkw Any progress on this one?

nimrodkor avatar Mar 15 '24 03:03 nimrodkor

Hey sorry. Nothing new. I hope I can make some progress tomorrow or next Thursday. I set myself a deadline for next weekend.

valkum avatar Mar 15 '24 17:03 valkum

@valkum all good 🙂 let me know, otherwise I will create a new PR branching off yours 💪

gruebel avatar Mar 15 '24 18:03 gruebel

I'm not entirely sure, but is this the same #2924 and #2925? This seems to be similar especially to the latter but the former seems to fix the same thing in a simpler way (unless there is a hidden issue with that approach).

Arguably this has been the first PR, but I'd just like to bring it to attention here.

mladedav avatar Apr 22 '24 15:04 mladedav

I'm not entirely sure, but is this the same #2924 and #2925? This seems to be similar especially to the latter but the former seems to fix the same thing in a simpler way (unless there is a hidden issue with that approach).

Arguably this has been the first PR, but I'd just like to bring it to attention here.

It fixes the same problem but uses peek instead of ahead which makes the implementation slightly less complex. I think my PR provides a bit more DX with the custom error message. Not sure if there is a drawback of using ahead instead of peek.

valkum avatar Apr 22 '24 16:04 valkum

@hds Mind taking another look?

nimrodkor avatar Apr 24 '24 09:04 nimrodkor