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.
I'll add an allow dead code then, hopefully CI will pass 🙏 |
Also I got some extra feedback about alsuren/robokite#1, I could tag you on tokio discord or DM you on discord whichever you prefer 👍 |
whatever works! |
Okay all CI checks pass 👍 |
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.
overall, this looks good. I mostly made some documentation and style suggestions.
in follow-up PRs, it would be really nice to:
- add more detailed documentation about how to use
valuable
with tracing in the top-leveltracing_core::field
module docs - add an example that actually uses
valuable
by implementing a visitor or extracting specific values, which we don't currently demonstrate anywhere - consider adding valuable support to the json formatter for recording nested structs as JSON
…acing into feature/valuable-integration
The woes of trying to rush something in when tired
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
# 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
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: