-
Notifications
You must be signed in to change notification settings - Fork 847
core: record Valuable values as valuable::Value
#1881
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
Conversation
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.
|
cc @xd009642 |
davidbarsky
left a comment
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.
I think this is well-considered and I also think it requires the sign-off of others, but I'm happy to +1 this.
|
I wouldn't worry about the arg size thing. Taking |
|
wait, the trait method should have taken |
In #1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This was probably wrong, since `Value` is `Copy` and should only be a couple words, one of which might be a pointer...so it's probably better to just pass the `pointer + enum descriminant` (or `integer + enum descriminant`) instead of passing `pointer to (pointer + enum descriminant)`. This probably also makes the API a little nicer.
In #1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This was probably wrong, since `Value` is `Copy` and should only be a couple words, one of which might be a pointer...so it's probably better to just pass the `pointer + enum descriminant` (or `integer + enum descriminant`) instead of passing `pointer to (pointer + enum descriminant)`. This probably also makes the API a little nicer.
Depends on #1881 Right now, the `valuable` stuff isn't very discoverable --- enabling the feature just adds some trait impls and stuff that aren't particularly visible in the documentation. This PR adds some top-level docs on using `valuable`. In particular: - Added a section to the `tracing` and `tracing-core` lib.rs docs explaining the unstable features versioning policy and how to turn on unstable features - Added a section in the `field` module that explains how to use `valuable` to record fields. - It turns out the `tracing::field` module didn't really have docs, since it doesn't re-export the `tracing_core::field` module but re-exports its _types_ in a new module (because it adds a trait). It had a single line of docs that just said something about "structured key-value data". I fixed this by coping the docs from `tracing-core`. :/ - Enabled unstable features in the documentation on docs.rs and netlify.
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.
In tokio-rs#1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This was probably wrong, since `Value` is `Copy` and should only be a couple words, one of which might be a pointer...so it's probably better to just pass the `pointer + enum descriminant` (or `integer + enum descriminant`) instead of passing `pointer to (pointer + enum descriminant)`. This probably also makes the API a little nicer.
Depends on tokio-rs#1881 Right now, the `valuable` stuff isn't very discoverable --- enabling the feature just adds some trait impls and stuff that aren't particularly visible in the documentation. This PR adds some top-level docs on using `valuable`. In particular: - Added a section to the `tracing` and `tracing-core` lib.rs docs explaining the unstable features versioning policy and how to turn on unstable features - Added a section in the `field` module that explains how to use `valuable` to record fields. - It turns out the `tracing::field` module didn't really have docs, since it doesn't re-export the `tracing_core::field` module but re-exports its _types_ in a new module (because it adds a trait). It had a single line of docs that just said something about "structured key-value data". I fixed this by coping the docs from `tracing-core`. :/ - Enabled unstable features in the documentation on docs.rs and netlify.
This changes the
valuableintegration so thatrecord_valuetakesinstances of
valuable::Value<'_>, rather than&dyn valuable::Valuabletrait objects. The primary advantage of this is thata
Valuecan be produced by callingValuable::as_value, so it allowsusers to write code like this:
rather than this:
which feels a bit more ergonomic. It also simplifies the code in
tracing-core, since we no longer need our ownValuableValuewrappertype 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, althoughthat's probably not as big a deal.
I didn't remove the
field::valuablefunction, as I thought it was niceto have for consistency with the existing
field::debugandfield::displayfunctions.Performance Considerations
@carllerche pointed out that a
Value<'_>might be slightly more bytesto pass on the stack than a trait object (always two words). I believe
this is only the case when the
Valueis aListable,Enumerable,Structable,Mappable, orTupleable, where theValuewould be anenum descriminant and a wide pointer to a trait object. However, in
the cases where the value is a primitive,
Valuewill be two words ifthe primitive is word-sized (e.g.
u64on 64-bit platforms), for theenum descriminant + the value, or one word if the primitive is smaller
than word size (
bool,char, etc). Also, for primitiveValues,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
visiton the value. However, if thetracingvisitor is goingto call
as_value()on the recorded value, this approach is better,because calling
as_value()in the macro prior to recording thespan/event will use the statically dispatched
as_value()impl on aknown type, rather than the the dynamically dispatched
as_value()implon the trait object. Since
as_valueimpls 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.