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

trace: Determine new syntax for uninitialized fields #77

Closed
hawkw opened this issue Jun 7, 2019 · 5 comments · Fixed by #548
Closed

trace: Determine new syntax for uninitialized fields #77

hawkw opened this issue Jun 7, 2019 · 5 comments · Fixed by #548
Assignees
Labels
crate/tracing Related to the `tracing` crate kind/rfc A request for comments to discuss future changes needs/design Additional design discussion is required.

Comments

@hawkw
Copy link
Member

hawkw commented Jun 7, 2019

Followup from #1062 and tokio-rs/tokio#1103 (comment).

Prior to #1103, the syntax field, was used by the tokio-trace macros to indicate span fields which have yet to have a value recorded. In order to support shorthand for using local variables as fields, we changed the meaning of this syntax, since local variable shorthand is likely to be a more common use-case (see #1103).

This leaves uninitialized fields without an appropriate macro syntax. There have been some suggestions, of which I think the strongest thus far are let field, and field = _,.

let field nicely mirrors the way let bindings with unset values are created in Rust; it does, however, seem somewhat inconsistent with the existing syntax, since other fields do not begin with let. On the other hand, field = _ fits in with the existing field syntax, but @carllerche has pointed out that it uses the assignment syntax to indicate something which is unassigned, which seems confusing.

We should try to agree on a new syntax for uninitialized fields, and re-enable support for them in the macros.

@hawkw
Copy link
Member Author

hawkw commented Jun 7, 2019

Personally, I think let field would be fine, but I'd like to hear from everyone who's invested in this.

hawkw referenced this issue in tokio-rs/tokio Jun 9, 2019
## Motivation

A common pattern in `tokio-trace` is to use the value of a local
variable as a field on a span or event. Currently, this requires code
like:
```rust
info!(foo = foo);
```
which is not particularly ergonomic given how commonly this occurs.
Struct initializers support a shorthand syntax for fields where the name
of the field is the same as a local variable, and `tokio-trace` should
as well.

## Solution

This branch adds support for syntax like
```rust
let foo = ...;
info!(foo);
```
and 
```rust
let foo = Foo {
    bar: ...,
    ...
};
info!(foo.bar)
```
to the `tokio-trace` span and event macros. This syntax also works with
the `Debug` and `Display` field shorthand.

The span macros previously used a field name with no value to indicate 
an uninitialized field. A new issue, #1138, has been opened for finding a
replacement syntax for uninitialized fields. Until then, the `tokio-trace` 
macros will no longer provide a way to create fields without values, 
although the `-core` API will continue to support this.

Closes #1062 

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Jun 12, 2019

@jonhoo, @carllerche, @davidbarsky, and @LucioFranco, just checking to see if supporting uninitialized fields in tokio-trace 0.2 is important to any of you. If so, we'll block the breaking change on this issue. Otherwise, when we agree on the new syntax, we can add it in a small-point release...

@davidbarsky
Copy link
Member

@hawkw It's not a blocking issue for me. I think it can be added later.

@LucioFranco
Copy link
Member

Same 😄

@carllerche carllerche transferred this issue from tokio-rs/tokio Jun 24, 2019
@hawkw hawkw added the crate/tracing Related to the `tracing` crate label Jun 24, 2019
@hawkw hawkw added the kind/rfc A request for comments to discuss future changes label Jul 10, 2019
@hawkw hawkw added the needs/design Additional design discussion is required. label Aug 5, 2019
@davidbarsky
Copy link
Member

I thought about this a bit more and I think I prefer field = _ despite the possible confusion with assignment syntax. The field = _ syntax reminds of anonymous lifetimes ('_) and placeholders in generics (Vec<_>) much more than the fact we're not assigning values to fields.

hawkw added a commit that referenced this issue Jan 27, 2020
Right now, there is no way to indicate that a span field _currently_
has no value but that a value will be recorded later. Tracing used to
support this concept, but when we added local-variable shorthand, it was
removed from the macros, as we needed to use the previous syntax for
indicating empty fields for local variable shorthand. See #77 for
details.

This commit side-steps the problem of determining an appropriate
_syntax_ for empty fields by adding a special value _type_, called
`Empty`. Now, empty values can be indicated like
```rust
span!("my_span", foo = 5, bar = tracing::field::Empty);
```
The `Empty` type implements `Value`, but its' `record` method is a no-op
that doesn't' call any functions on the visitor. Therefore, when a field
is empty, the visitor will not see it. An empty field can then be
recorded later using `Subscriber::record` (or the corresponding `Span`
method in `tracing`).

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw closed this as completed in #548 Feb 6, 2020
hawkw added a commit that referenced this issue Feb 6, 2020
Right now, there is no way to indicate that a span field _currently_
has no value but that a value will be recorded later. Tracing used to
support this concept, but when we added local-variable shorthand, it was
removed from the macros, as we needed to use the previous syntax for
indicating empty fields for local variable shorthand. See #77 for
details.

This commit side-steps the problem of determining an appropriate
_syntax_ for empty fields by adding a special value _type_, called
`Empty`. Now, empty values can be indicated like
```rust
span!("my_span", foo = 5, bar = tracing::field::Empty);
```
The `Empty` type implements `Value`, but its' `record` method is a no-op
that doesn't' call any functions on the visitor. Therefore, when a field
is empty, the visitor will not see it. An empty field can then be
recorded later using `Subscriber::record` (or the corresponding `Span`
method in `tracing`).

Closes #77 

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/tracing Related to the `tracing` crate kind/rfc A request for comments to discuss future changes needs/design Additional design discussion is required.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants