Skip to content

Commit

Permalink
core: record Valuable values as valuable::Value (#1881)
Browse files Browse the repository at this point in the history
This changes the `valuable` integration so that `record_value` takes
instances of `valuable::Value<'_>`, rather than `&dyn
valuable::Valuable` trait objects. The primary advantage of this is that
a `Value` can be produced by calling `Valuable::as_value`, so it allows
users to write code like this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = foo.as_value());
```

rather than this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = tracing::field::valuable(&foo));
```

which feels a bit more ergonomic. It also simplifies the code in
`tracing-core`, since we no longer need our own `ValuableValue` wrapper
type to turn things into trait objects.

It might also reduce boilerplate a bit on the visitor side, as
`as_value()` doesn't have to be called on the trait object, although
that's probably not as big a deal.

I didn't remove the `field::valuable` function, as I thought it was nice
to have for consistency with the existing `field::debug` and
`field::display` functions.

## Performance Considerations

@carllerche pointed out that a `Value<'_>` might be slightly more bytes
to pass on the stack than a trait object (always two words). I believe
this is only the case when the `Value` is a `Listable`, `Enumerable`,
`Structable`, `Mappable`, or `Tupleable`, where the `Value` would be an
enum descriminant _and_ a wide pointer to a trait object. However, in
the cases where the value is a primitive, `Value` will be two words if
the primitive is word-sized (e.g. `u64` on 64-bit platforms), for the
enum descriminant + the value, or one word if the primitive is smaller
than word size (`bool`, `char`, etc). Also, for primitive `Value`s,
there's no pointer dereference, which the trait object always requires.

I'm not sure how the enum dispatch compares to vtable dispatch when
calling `visit` on the value. However, if the `tracing` visitor is going
to call `as_value()` on the recorded value, this approach is better,
because calling `as_value()` in the macro _prior_ to recording the
span/event will use the statically dispatched `as_value()` impl on a
known type, rather than the the dynamically dispatched `as_value()` impl
on the trait object. Since `as_value` impls are generally quite trivial,
I'd guess they usually (always?) will get inlined, which is never
possible with the dynamically dispatched call after passing a trait
object into `tracing`.

In practice I'm not sure if there's a huge perf diff either way, but it
was interesting to think through the implications.
  • Loading branch information
hawkw authored Feb 2, 2022
1 parent 2c3555b commit 532b61c
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 20 deletions.
4 changes: 1 addition & 3 deletions examples/examples/valuable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
//!
//! Therefore, when `valuable` support is not enabled, this example falls back to using
//! `fmt::Debug` to record fields that implement `valuable::Valuable`.
#[cfg(tracing_unstable)]
use tracing::field::valuable;
use tracing::{info, info_span};
use valuable::Valuable;

Expand Down Expand Up @@ -49,7 +47,7 @@ fn main() {
// If the `valuable` feature is enabled, record `user` using its'
// `valuable::Valuable` implementation:
#[cfg(tracing_unstable)]
let span = info_span!("Processing", user = valuable(&user));
let span = info_span!("Processing", user = user.as_value());

// Otherwise, record `user` using its `fmt::Debug` implementation:
#[cfg(not(tracing_unstable))]
Expand Down
29 changes: 12 additions & 17 deletions tracing-core/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ pub trait Visit {
/// [`valuable`]: https://docs.rs/valuable
#[cfg(all(tracing_unstable, feature = "valuable"))]
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))]
fn record_value(&mut self, field: &Field, value: &dyn valuable::Valuable) {
fn record_value(&mut self, field: &Field, value: &valuable::Value<'_>) {
self.record_debug(field, &value)
}

Expand Down Expand Up @@ -258,14 +258,6 @@ pub struct DisplayValue<T: fmt::Display>(T);
#[derive(Clone)]
pub struct DebugValue<T: fmt::Debug>(T);

/// A `Value` which serializes using [`Valuable`].
///
/// [`Valuable`]: https://docs.rs/valuable/latest/valuable/trait.Valuable.html
#[derive(Clone)]
#[cfg(all(tracing_unstable, feature = "valuable"))]
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))]
pub struct ValuableValue<T: valuable::Valuable>(T);

/// Wraps a type implementing `fmt::Display` as a `Value` that can be
/// recorded using its `Display` implementation.
pub fn display<T>(t: T) -> DisplayValue<T>
Expand All @@ -290,11 +282,11 @@ where
/// [`Valuable`]: https://docs.rs/valuable/latest/valuable/trait.Valuable.html
#[cfg(all(tracing_unstable, feature = "valuable"))]
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))]
pub fn valuable<T>(t: T) -> ValuableValue<T>
pub fn valuable<T>(t: &T) -> valuable::Value<'_>
where
T: valuable::Valuable,
{
ValuableValue(t)
t.as_value()
}

// ===== impl Visit =====
Expand Down Expand Up @@ -572,21 +564,24 @@ impl<T: fmt::Debug> fmt::Debug for DebugValue<T> {
// ===== impl ValuableValue =====

#[cfg(all(tracing_unstable, feature = "valuable"))]
impl<T: valuable::Valuable> crate::sealed::Sealed for ValuableValue<T> {}
impl crate::sealed::Sealed for valuable::Value<'_> {}

#[cfg(all(tracing_unstable, feature = "valuable"))]
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))]
impl<T: valuable::Valuable> Value for ValuableValue<T> {
impl Value for valuable::Value<'_> {
fn record(&self, key: &Field, visitor: &mut dyn Visit) {
visitor.record_value(key, &self.0)
visitor.record_value(key, self)
}
}

#[cfg(all(tracing_unstable, feature = "valuable"))]
impl crate::sealed::Sealed for &'_ dyn valuable::Valuable {}

#[cfg(all(tracing_unstable, feature = "valuable"))]
#[cfg_attr(docsrs, doc(cfg(all(tracing_unstable, feature = "valuable"))))]
impl<T: valuable::Valuable> fmt::Debug for ValuableValue<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{:?}", &self.0 as &dyn valuable::Valuable)
impl Value for &'_ dyn valuable::Valuable {
fn record(&self, key: &Field, visitor: &mut dyn Visit) {
visitor.record_value(key, &self.as_value())
}
}

Expand Down

0 comments on commit 532b61c

Please sign in to comment.