-
Notifications
You must be signed in to change notification settings - Fork 734
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
Feature/valuable integration #1608
Feature/valuable integration #1608
Conversation
5147657
to
7a2f3c5
Compare
Okay after some useful guidance from @hawkw on discord I've added a use valuable::Valuable;
use tracing::{info, info_span};
#[derive(Valuable)]
pub struct Person {
name: String,
age: u32,
addresses: Vec<Address>,
}
#[derive(Valuable)]
pub struct Address {
street: String,
city: String,
zip: String,
}
fn blah(value: &(dyn Valuable + 'static)) -> tracing::Span {
info_span!("span", v=tracing::field::valuable(value))
}
fn blahblah(person: &Person) -> tracing::Span {
info_span!("span", v=tracing::field::valuable(person))
}
fn main() {
let person = Person {
name: "Daniel".to_string(),
age: 28,
addresses: vec![
Address {
street: "redacted".to_string(),
city: "London".to_string(),
zip: "redacted".to_string()
}
]
};
blah(&person);
info_span!("Greeting", person=tracing::field::valuable(person));
info!("Hello");
} I haven't yet made any moves to gate it behind a cfg like |
Regardless of whether or not we add an unstable cfg, it should be an optional dependency. The goal is that the |
I'm going to try to give this a closer review this afternoon. However, I wanted to start by saying that we are not going to be able to merge this PR until there's a crates.io release of Therefore, since this is currently blocked on a published cc @carllerche |
I've just added the config, missing debug implementation and missing doc comments for this PR. Pending any review comments and a valuable release to crates.io the only thing I can see that would need doing is an update to the CI so the valuable code is also tested. But not sure how thorough the checks should be (i.e. do we still care about MSRV for an unstable feature). If I have time to do it I'll also work on playing around using this branch on a personal project and if anything of interest comes up or I push anything publicly I'll mention it 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I finally had a chance to give this a review, and overall it looks great. I commented on a few small things that I think we ought to change.
It would definitely also be nice to have some tests for this, and to flesh out the docs and examples somewhat, but that isn't strictly necessary until we can actually merge this PR (which is still blocked on an actual release of valuable
). So, we can do that later --- for now, I think testing out the branch to make sure it's actually usable is probably the most important next step!
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))] | ||
impl<T: valuable::Valuable> Value for ValuableValue<T> { | ||
fn record(&self, key: &Field, visitor: &mut dyn Visit) { | ||
visitor.record_value(key, &self.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a potential thought, although i'm not sure whether or not it's necessary: valuable
's list of primitive types has some overlaps with tracing
's. we may want to consider doing something where we call Valuable::as_value
, match on the result, and call the Visit
type's record_$TY
methods for tracing primitive types if the value is a tracing primitive? e.g.
visitor.record_value(key, &self.0) | |
match self.0.as_value() { | |
valuable::Value::I64(v) => visitor.record_i64(key, v), | |
valuable::Value::U64(v) => visitor.record_u64(key, v), | |
// ... | |
_ => visitor.record_value(key, &self.0) | |
} |
I'm not totally sure if this is the right approach or not, but the thought process here is that visitors that are aware of Valuable
will probably still implement the record_$TY
methods for various tracing primitives as well, since a majority of the values recorded will not be Valuable
s. but, the visitor implementation could always handle the duplicated code itself...idk if this is worth doing here or not. probably worth testing it out in your own code and seeing how it feels...
Oh yeah tests would be good, I guess I'll work out something with a custom visitor at some point to make sure the dispatch works correctly - unless there's any smart/more-important things to test with this. When I have some code using this branch I'll apply the unresolved suggested change and see what it's like with and without it |
Another good way to test out that this actually works correctly would be writing a few quick examples in the |
Added a very simple example, emphasis on very simple it just derives Clippy doesn't like the example without a main function though, so maybe attributes on everything is a nicer approach just for CI |
hmm, that's annoying. another option is to make a separate crate it may also be worth looking at what other crates in the ecosystem do for examples that require a cfg to be set? as a side note, it looks like it's not just clippy; a missing |
I just realized that this branch is based on I tried changing the base in the github UI, but this branch also contains a bunch of unrelated changes from |
Added the missing Debug implementation for `ValuableValue` and put the valuable integration behind a tracing_unstable config as well.
…acing into feature/valuable-integration
The woes of trying to rush something in when tired
Co-authored-by: Eliza Weisman <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
# 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!
# 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
# 0.1.30 (February 3rd, 2021) 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. This release also adds a new `enabled!` macro for testing if a span or event would be enabled. ### Added - **field**: Experimental support for recording field values using the [`valuable`] crate ([#1608], [#1888], [#1887]) - `enabled!` macro for testing if a span or event is enabled ([#1882]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] - `tracing-attributes`: updated to [0.1.19][attributes-0.1.19] ### Fixed - **log**: Fixed "use of moved value" compiler error when the "log" feature is enabled ([#1823]) - Fixed macro hygiene issues when used in a crate that defines its own `concat!` macro ([#1842]) - A very large number of documentation fixes and improvements. Thanks to @@vlad-scherbina, @Skepfyr, @Swatinem, @guswynn, @teohhanhui, @xd009642, @tobz, @d-e-s-o@0b01, and @nickelc for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [attributes-0.1.19]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.19 [#1608]: #1608 [#1888]: #1888 [#1887]: #1887 [#1882]: #1882 [#1823]: #1823 [#1842]: #1842
# 0.1.30 (February 3rd, 2021) 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. This release also adds a new `enabled!` macro for testing if a span or event would be enabled. ### Added - **field**: Experimental support for recording field values using the [`valuable`] crate ([#1608], [#1888], [#1887]) - `enabled!` macro for testing if a span or event is enabled ([#1882]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] - `tracing-attributes`: updated to [0.1.19][attributes-0.1.19] ### Fixed - **log**: Fixed "use of moved value" compiler error when the "log" feature is enabled ([#1823]) - Fixed macro hygiene issues when used in a crate that defines its own `concat!` macro ([#1842]) - A very large number of documentation fixes and improvements. Thanks to @@vlad-scherbina, @Skepfyr, @Swatinem, @guswynn, @teohhanhui, @xd009642, @tobz, @d-e-s-o@0b01, and @nickelc for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [attributes-0.1.19]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.19 [#1608]: #1608 [#1888]: #1888 [#1887]: #1887 [#1882]: #1882 [#1823]: #1823 [#1842]: #1842
# 0.3.4 (Dec 23, 2021) This release contains bugfixes for the `fmt` module, as well as documentation improvements. ### Fixed - **fmt**: Fixed `fmt` not emitting log lines when timestamp formatting fails ([tokio-rs#1689]) - **fmt**: Fixed double space before thread IDs with `Pretty` formatter ([tokio-rs#1778]) - Several documentation improvements ([tokio-rs#1608], [tokio-rs#1699], [tokio-rs#1701]) [tokio-rs#1689]: tokio-rs#1689 [tokio-rs#1778]: tokio-rs#1778 [tokio-rs#1608]: tokio-rs#1608 [tokio-rs#1699]: tokio-rs#1699 [tokio-rs#1701]: tokio-rs#1701 Thanks to new contributors @Swatinem and @rukai for contributing to this release!
This branch adds initial support for using the [`valuable`] crate as an opt-in `Value` type in `tracing`. `valuable` provides a mechanism for defining custom ways to record user-implemented types, as well as structured recording of standard library data structures such as maps, arrays, and sets. For more details, see the tracking issue tokio-rs#1570. In `tracing` v0.2, the intent is for `valuable` to replace the existing `tracing_core::field::Value` trait. However, in v0.1.x, `valuable` support must be added in a backwards-compatible way, so recording types that implement `valuable::Valueable` currently requires opting in using a `field::valuable` wrapper function. Since this is the first release of `valuable` and the API is still stabilizing, we don't want to tie `tracing-core`'s stability to `valuable`'s. Therefore, the valuable dependency is feature-flagged *and* also requires `RUSTFLAGS="--cfg tracing_unstable"`. [`valuable`]: https://github.com/tokio-rs/valuable Co-authored-by: Daniel McKenna <[email protected]> Co-authored-by: David Barsky <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
# 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
# 0.1.30 (February 3rd, 2021) 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. This release also adds a new `enabled!` macro for testing if a span or event would be enabled. ### Added - **field**: Experimental support for recording field values using the [`valuable`] crate ([tokio-rs#1608], [tokio-rs#1888], [tokio-rs#1887]) - `enabled!` macro for testing if a span or event is enabled ([tokio-rs#1882]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] - `tracing-attributes`: updated to [0.1.19][attributes-0.1.19] ### Fixed - **log**: Fixed "use of moved value" compiler error when the "log" feature is enabled ([tokio-rs#1823]) - Fixed macro hygiene issues when used in a crate that defines its own `concat!` macro ([tokio-rs#1842]) - A very large number of documentation fixes and improvements. Thanks to @@vlad-scherbina, @Skepfyr, @Swatinem, @guswynn, @teohhanhui, @xd009642, @tobz, @d-e-s-o@0b01, and @nickelc for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [attributes-0.1.19]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.19 [tokio-rs#1608]: tokio-rs#1608 [tokio-rs#1888]: tokio-rs#1888 [tokio-rs#1887]: tokio-rs#1887 [tokio-rs#1882]: tokio-rs#1882 [tokio-rs#1823]: tokio-rs#1823 [tokio-rs#1842]: tokio-rs#1842
@@ -27,7 +27,7 @@ edition = "2018" | |||
rust-version = "1.42.0" | |||
|
|||
[features] | |||
default = ["std"] | |||
default = ["std", "valuable/std"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this causes cargo to download the valuable
crate even though it isn't actually used without tracing_unstable
…`valuable` is used (#3002) ## Motivation `tracing-core` adds a dependency on `valuable` anytime the `std` (or `default`) feature is enabled even when `valuable` is not meant to be used. This was pointed out by [this comment](#1608 (comment)). ## Solution Only add the feature dependency when something enables `valuable`. This does not apply to `master` as `valuable` support there is always on.
Start the initial integration of Valuable into tracing by adding it to the visit trait, implementing Value for Valuable and seeing if it all works. For further motivation see the tracking issue: #1570
For my own testing, I have a project with the following main.rs, I grabbed the Person and Address types from a valuable example:
Currently
blah
works, but blahblah has a compilation error which I'm currently trying to resolve but I rarely touch macros in rust so any help would be appreciated: