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

core: change metadata::Kind to a bitflag #1891

Merged
merged 2 commits into from
Feb 3, 2022
Merged

core: change metadata::Kind to a bitflag #1891

merged 2 commits into from
Feb 3, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Feb 3, 2022

This changes the Kind type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the enabled! macro that specifically
check if a span would be enabled, or if an event would be enabled.

See this comment for details.

This does not actually implement the enabled! variants, just the
Kind representation change. This way, we can add to the enabled!
macro in a subsequent tracing release without having to change
tracing-core again.

I went with the bitflag representation rather than adding a bool to the
KindInner::Span and KindInner::Event enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.
@hawkw hawkw requested a review from a team February 3, 2022 18:21
@hawkw hawkw requested a review from carllerche as a code owner February 3, 2022 18:21
@hawkw hawkw merged commit c27f0fd into master Feb 3, 2022
@hawkw hawkw deleted the eliza/hint_kinds branch February 3, 2022 18:47
Copy link
Contributor

@guswynn guswynn left a comment

Choose a reason for hiding this comment

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

this is awesome, thanks!

hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 4, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 4, 2022
# 0.1.22 (February 3, 2022)

This release adds *experimental* support for recording structured field
values using the [`valuable`] crate. See [this blog post][post] for
details on `valuable`.

Note that `valuable` support currently requires `--cfg
tracing_unstable`. See the documentation for details.

### Added

- **field**: Experimental support for recording field values using the
  [`valuable`] crate ([#1608], [#1888], [#1887])
- **field**: Added `ValueSet::record` method ([#1823])
- **subscriber**: `Default` impl for `NoSubscriber` ([#1785])
- **metadata**: New `Kind::HINT` to support the `enabled!` macro in
  `tracing` ([#1883], [#1891])
### Fixed

- Fixed a number of documentation issues ([#1665], [#1692], [#1737])

Thanks to @xd009642, @Skepfyr, @guswynn, @Folyd, and @mbergkvist for
contributing to this release!
hawkw added a commit that referenced this pull request Feb 4, 2022
# 0.1.22 (February 3, 2022)

This release adds *experimental* support for recording structured field
values using the [`valuable`] crate. See [this blog post][post] for
details on `valuable`.

Note that `valuable` support currently requires `--cfg
tracing_unstable`. See the documentation for details.

### Added

- **field**: Experimental support for recording field values using the
  [`valuable`] crate ([#1608], [#1888], [#1887])
- **field**: Added `ValueSet::record` method ([#1823])
- **subscriber**: `Default` impl for `NoSubscriber` ([#1785])
- **metadata**: New `Kind::HINT` to support the `enabled!` macro in
  `tracing` ([#1883], [#1891])
### Fixed

- Fixed a number of documentation issues ([#1665], [#1692], [#1737])

Thanks to @xd009642, @Skepfyr, @guswynn, @Folyd, and @mbergkvist for
contributing to this release!

[`valuable`]: https://crates.io/crates/valuable
[post]: https://tokio.rs/blog/2021-05-valuable
[#1608]: #1608
[#1888]: #1888
[#1887]: #1887
[#1823]: #1823
[#1785]: #1785
[#1883]: #1883
[#1891]: #1891
[#1665]: #1665
[#1692]: #1692
[#1737]: #1737
@kornelski
Copy link
Contributor

This broke tokio-rs/console#270

@nagisa
Copy link
Contributor

nagisa commented Feb 10, 2022

This also broke standard-ai/tracing-gstreamer#6. I’ll be fixing this on my side but I wanted to make a note and give a heads-up:

In tokio-rs/console#270 (comment) @hawkw wrote:

In this case, I am fairly sure that the console is approximately the only code that matched on this type, since there should be no publicly accessible way for user code to actually get a Kind from a Metadata

While there is no public way to obtain Kind from a Metadata, nothing prevented code from doing something along the lines of spelling out Kind::EVENT to construct a Kind and then matching on that value later.

That said, I'm quite sympathetic to the fact that the exhaustiveness was unintentional.

@hawkw
Copy link
Member Author

hawkw commented Feb 10, 2022

@nagisa Ugh, sorry about that. I went back and forth for a while on whether this release should be yanked, and I was hoping that the blast radius was small enough that not yanking it was an acceptable decision, because exhaustive matching was never intended here.

I hope this didn't cause you too much of an inconvenience!

@nagisa
Copy link
Contributor

nagisa commented Feb 10, 2022

tracing-gstreamer in particular is not very widely used and doesn't expose Kind in its public API making it fairly straightforward to address it on my side. I'm not sure if there will be any other affected crates, but tracing-gstreamer is quite unconventional implementation of a subscriber, so hopefully there won't be any!

kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: tokio-rs#1821 (comment)
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.22 (February 3, 2022)

This release adds *experimental* support for recording structured field
values using the [`valuable`] crate. See [this blog post][post] for
details on `valuable`.

Note that `valuable` support currently requires `--cfg
tracing_unstable`. See the documentation for details.

### Added

- **field**: Experimental support for recording field values using the
  [`valuable`] crate ([tokio-rs#1608], [tokio-rs#1888], [tokio-rs#1887])
- **field**: Added `ValueSet::record` method ([tokio-rs#1823])
- **subscriber**: `Default` impl for `NoSubscriber` ([tokio-rs#1785])
- **metadata**: New `Kind::HINT` to support the `enabled!` macro in
  `tracing` ([tokio-rs#1883], [tokio-rs#1891])
### Fixed

- Fixed a number of documentation issues ([tokio-rs#1665], [tokio-rs#1692], [tokio-rs#1737])

Thanks to @xd009642, @Skepfyr, @guswynn, @Folyd, and @mbergkvist for
contributing to this release!

[`valuable`]: https://crates.io/crates/valuable
[post]: https://tokio.rs/blog/2021-05-valuable
[tokio-rs#1608]: tokio-rs#1608
[tokio-rs#1888]: tokio-rs#1888
[tokio-rs#1887]: tokio-rs#1887
[tokio-rs#1823]: tokio-rs#1823
[tokio-rs#1785]: tokio-rs#1785
[tokio-rs#1883]: tokio-rs#1883
[tokio-rs#1891]: tokio-rs#1891
[tokio-rs#1665]: tokio-rs#1665
[tokio-rs#1692]: tokio-rs#1692
[tokio-rs#1737]: tokio-rs#1737
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.

5 participants