Allow string literal field identifiers

[valkum]

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.

[gruebel]

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?

[valkum]

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?

[nimrodkor]

@valkum @hawkw Any progress on this one?

[valkum]

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

[gruebel]

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

[hds]

Thank you very much for the PR and sorry for the long lead time on the review.

We're happy with the change, but ideally we'd like a test for the negative case (compilation error I'm guessing) mentioned in the review comments.

Given how long it's been, if you don't have the time and/or interest to come back to this PR, please let us know and we'll hand it off to someone else.

Only a PR against master is necessary, we'll handle backporting ourselves.

Once again, thank you very much!

[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.

[valkum]

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.

[nimrodkor]

@hds Mind taking another look?

[hds]

This looks good to me now, thank you!

@davidbarsky what do you think?

[nimrodkor]

Any updates here? @davidbarsky @valkum we've been running our own fork for a long time waiting for this 🙏