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

Allow owned values and fat pointers in Span::record #2212

Merged
merged 2 commits into from Jul 13, 2022

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Jul 8, 2022

Motivation

Previously, using record("x", "y") would give an error:

error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> src/main.rs:3:22
     |
243  |                 span.record("x", "y");
     |                      ^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `Span::record`
    --> /home/jnelson/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.32/src/span.rs:1184:30
     |
1184 |     pub fn record<Q: ?Sized, V>(&self, field: &Q, value: &V) -> &Self
     |                              ^ required by this bound in `Span::record`

Solution

Take Q: Value directly instead of Q: &Value. This doesn't break any existing code, because there's a generic impl<T: Value> Value for &T: https://docs.rs/tracing/0.1.35/tracing/trait.Value.html#impl-Value-for-%26%27a%20T

There's a simpler change which is just to remove the implicit Sized restriction on Q, but that still wouldn't allow passing false instead of &false.

@jyn514 jyn514 requested review from hawkw, davidbarsky and a team as code owners July 8, 2022 16:42
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, this looks good to me!

Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

approved, but i'm unreasonably paranoid that the previous impls don't work. not blocking: can you maybe add a unit test for the previous, ref-based usage?

Previously, using `record("x", "y")` would give an error:
```
error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> src/main.rs:3:22
     |
243  |                 span.record("x", "y");
     |                      ^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `Span::record`
    --> /home/jnelson/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.32/src/span.rs:1184:30
     |
1184 |     pub fn record<Q: ?Sized, V>(&self, field: &Q, value: &V) -> &Self
     |                              ^ required by this bound in `Span::record`
```

Now it works fine, as tested by the doc-example.

This doesn't break any existing code, because there's a generic `impl<T: Value> Value for &T`: https://docs.rs/tracing/0.1.35/tracing/trait.Value.html#impl-Value-for-%26%27a%20T
@jyn514
Copy link
Contributor Author

jyn514 commented Jul 13, 2022

Sure thing, done :)

@hawkw hawkw enabled auto-merge (squash) July 13, 2022 23:13
@hawkw hawkw merged commit 4f326d2 into tokio-rs:master Jul 13, 2022
@jyn514 jyn514 deleted the no-multi-ref branch July 13, 2022 23:31
hawkw added a commit that referenced this pull request Jul 20, 2022
Previously, using `record("x", "y")` would give an error:
```
error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> src/main.rs:3:22
     |
243  |                 span.record("x", "y");
     |                      ^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `Span::record`
    --> /home/jnelson/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.32/src/span.rs:1184:30
     |
1184 |     pub fn record<Q: ?Sized, V>(&self, field: &Q, value: &V) -> &Self
     |                              ^ required by this bound in `Span::record`
```

Now it works fine, as tested by the doc-example.

This doesn't break any existing code, because there's a generic `impl<T: Value> Value for &T`: https://docs.rs/tracing/0.1.35/tracing/trait.Value.html#impl-Value-for-%26%27a%20T

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 20, 2022
Previously, using `record("x", "y")` would give an error:
```
error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> src/main.rs:3:22
     |
243  |                 span.record("x", "y");
     |                      ^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `Span::record`
    --> /home/jnelson/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.32/src/span.rs:1184:30
     |
1184 |     pub fn record<Q: ?Sized, V>(&self, field: &Q, value: &V) -> &Self
     |                              ^ required by this bound in `Span::record`
```

Now it works fine, as tested by the doc-example.

This doesn't break any existing code, because there's a generic `impl<T: Value> Value for &T`: https://docs.rs/tracing/0.1.35/tracing/trait.Value.html#impl-Value-for-%26%27a%20T

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 20, 2022
Previously, using `record("x", "y")` would give an error:
```
error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> src/main.rs:3:22
     |
243  |                 span.record("x", "y");
     |                      ^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `Span::record`
    --> /home/jnelson/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.32/src/span.rs:1184:30
     |
1184 |     pub fn record<Q: ?Sized, V>(&self, field: &Q, value: &V) -> &Self
     |                              ^ required by this bound in `Span::record`
```

Now it works fine, as tested by the doc-example.

This doesn't break any existing code, because there's a generic `impl<T: Value> Value for &T`: https://docs.rs/tracing/0.1.35/tracing/trait.Value.html#impl-Value-for-%26%27a%20T

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 29, 2022
# 0.1.36 (July 29, 2022)

This release adds support for owned values and fat pointers as arguments
to the `Span::record` method, as well as updating the minimum
`tracing-core` version and several documentation improvements.

### Fixed

- Incorrect docs in `dispatcher::set_default` ([#2220])
- Compilation with `-Z minimal-versions` ([#2246])

### Added

- Support for owned values and fat pointers in `Span::record` ([#2212])
- Documentation improvements ([#2208], [#2163])

### Changed

- `tracing-core`: updated to [0.1.29][core-0.1.29]

Thanks to @fredr, @cgbur, @jyn514, @matklad, and @CAD97 for contributing
to this release!

[core-0.1.29]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.29
[#2220]: #2220
[#2246]: #2246
[#2212]: #2212
[#2208]: #2208
[#2163]: #2163
hawkw added a commit that referenced this pull request Jul 29, 2022
# 0.1.36 (July 29, 2022)

This release adds support for owned values and fat pointers as arguments
to the `Span::record` method, as well as updating the minimum
`tracing-core` version and several documentation improvements.

### Fixed

- Incorrect docs in `dispatcher::set_default` ([#2220])
- Compilation with `-Z minimal-versions` ([#2246])

### Added

- Support for owned values and fat pointers in `Span::record` ([#2212])
- Documentation improvements ([#2208], [#2163])

### Changed

- `tracing-core`: updated to [0.1.29][core-0.1.29]

Thanks to @fredr, @cgbur, @jyn514, @matklad, and @CAD97 for contributing
to this release!

[core-0.1.29]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.29
[#2220]: #2220
[#2246]: #2246
[#2212]: #2212
[#2208]: #2208
[#2163]: #2163
caio added a commit to caio/foca that referenced this pull request Nov 9, 2022
tracing-attributes 0.1.23 ships with a smarter `Span::record` that
allows owned values [^1], which makes

This means that previously invalid code such as:

    span.record("num_updates", num_items);

Is now valid, which makes clippy throw a warning for needless_borrow

Fixing the lint, however, means that foca would require a strict version
check for tracing-attributes (otherwise anything pulling 0.1.22 or
bellow would not compile anymore).

I don't want a strict dependency on tracing: it's tangential do foca and
I'd like users to benefit from the improvements that might be shipped
along, so I fix the lint warning by ignoring it instead.

Once we get a (at least minor) version bump on `tracing`, this can be
removed and updated to skip the borrow.

[^1]: tokio-rs/tracing#2212
caio added a commit to caio/foca that referenced this pull request Nov 13, 2022
tracing-attributes 0.1.23 ships with a smarter `Span::record` that
allows owned values [^1], which makes

This means that previously invalid code such as:

    span.record("num_updates", num_items);

Is now valid, which makes clippy throw a warning for needless_borrow

Fixing the lint, however, means that foca would require a strict version
check for tracing-attributes (otherwise anything pulling 0.1.22 or
bellow would not compile anymore).

I don't want a strict dependency on tracing: it's tangential do foca and
I'd like users to benefit from the improvements that might be shipped
along, so I fix the lint warning by ignoring it instead.

Once we get a (at least minor) version bump on `tracing`, this can be
removed and updated to skip the borrow.

[^1]: tokio-rs/tracing#2212
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