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: tracking issue for valuable integration #1570

Open
hawkw opened this issue Sep 15, 2021 · 8 comments
Open

core: tracking issue for valuable integration #1570

hawkw opened this issue Sep 15, 2021 · 8 comments
Labels
crate/core Related to the `tracing-core` crate crate/tracing Related to the `tracing` crate kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.

Comments

@hawkw
Copy link
Member

hawkw commented Sep 15, 2021

Feature Request

Crates

  • tracing-core

Motivation

The valuable crate provides an extensible system for dynamic inspection of structured values. It was primarily implemented in order to extend tracing's value system with support for visiting nested values (such as a struct's fields). This would be a significant improvement relative to the current value system (see e.g. #819, #845, etc). However, in order to benefit from valuable, we have to actually integrate it into tracing.

Proposal

There are actually two separate problems we will want to solve: valuable support in tracing 0.1, and valuable support for the upcoming v0.2.

In 0.2

For 0.2, the solution will be fairly simple, but probably takes a lot of work to implement. The intent is to more or less completely replace the existing Value trait with valuable::Valuable. I think we would also want to replace the ValueSet type with something implementing valuable's abstraction for arrays, as well. We would then modify the macros to record key-value pairs using valuable, rather than using the tracing_core::field::Value trait, and update existing code to use valuable. Of course, we'll want to make sure it's possible to reimplement the basic tracing field behaviors (e.g. recording primitives) on top of valuable with no additional friction.

In 0.1

It's probably a good idea to implement the valuable integration in tracing 0.1 before performing a whole-sale replacement in 0.2 --- it's quite likely that we'll hit things in valuable that may need to be changed or improved in order to support use in tracing, and this may require breaking changes in valuable. Using valuable to completely replace the Value trait makes valuable a public API dependency, and breaking changes to valuable are breaking changes in tracing-core. Therefore, we ought to make sure any breaking changes to valuable are out of the way prior to replacing the entire value system in 0.2.

Thus, we probably want to add optional support for valuable in 0.1 first. Unlike replacing the entire Value trait, this would be a backwards-compatible addition to the existing Value system. We'd probably want to do this by adding a new Visit::record_valuable method where the visitor is passed a dyn Valuable, behind a feature flag. We'd also add a Value implementation for valuable::Value that dispatches to this method. Since valuable::Value implements Debug, this can always fall back to the Visit type's record_debug if the visitor doesn't opt in to valuable support.

Alternatives

We could skip adding valuable support in 0.1, and just do the wholesale replacement in 0.2. However, I don't think this is a great idea, since it means we don't get the opportunity for users to actually test out using valuable with tracing, and there might be unforseen issues that require breaking API changes in valuable. Also, I think the 0.1 version of optional valuable integration is probably fairly simple to implement, so it seems like something we might as well do.

@hawkw hawkw added kind/feature New feature or request crate/core Related to the `tracing-core` crate crate/tracing Related to the `tracing` crate meta/breaking This is a breaking change, and should wait until the next breaking release. labels Sep 15, 2021
@hawkw hawkw added this to the tracing-core 0.2 milestone Sep 15, 2021
@hawkw
Copy link
Member Author

hawkw commented Sep 15, 2021

cc @carllerche

@davidbarsky
Copy link
Member

(Sorry, I should've commented sooner.)

In terms of approaches, I really the idea of introducing Valuable into tracing 0.1 under a feature flag. As for the actual mechanism, I think that valuable should be gated under a RUSTFLAGS="--cfg tracing_unstable" feature flag and not a Cargo-based feature flag. I think that introducing valuable into tracing 0.1 without a feature flag could make sense as a mechanism to introduce forwards compatibility with tracing 0.2, but that should only happen after some confidence in valuable is established.

I'm also not opposed to introducing a versioned, Cargo-based feature flag for valuable some time after the initial introduction via the RUSTFLAGS approach.

hawkw added a commit that referenced this issue Jan 21, 2022
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 #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]>
@marmistrz
Copy link

marmistrz commented Apr 11, 2022

I have second thoughts about the approach implemented by @hawkw in #1608.

Before we start, let's make sure we're on the same page regarding the expectations: suppose that we have the following struct:


#[derive(Valuable)]
pub struct Person {
    name: String,
    age: u32,
    addresses: Vec<Address>,
}

then, given the following call:

info!(person = valuable(&person), "blah")

I'd expect a JSON output along these lines:

{
    "message": "blah",
    "person.name": "Alice",
    "person.age": 42,
    ...
}

while the current output is along these lines:

{
    "message": "blah",
    "person": "Person { name: "Alice",  age: 42, ... }"
    ...
}

This happens because the current approach simply uses the Debug implementation to record the value:
https://github.com/xd009642/tracing/blob/062a6ea6ec371c26d9fa8fa7b1651a5c41f9e67e/tracing-core/src/field.rs#L193
but for this we don't even need any valuable integration. And this seems wrong.

I have an impression that we should call field.visit(self) instead (or something along these lines), so that the Valuable has the opportunity to separately record all of its fields.

@hawkw
Copy link
Member Author

hawkw commented Apr 11, 2022

This happens because the current approach simply uses the Debug implementation to record the value: https://github.com/xd009642/tracing/blob/062a6ea6ec371c26d9fa8fa7b1651a5c41f9e67e/tracing-core/src/field.rs#L193 but for this we don't even need any valuable integration. And this seems wrong.

This is the default implementation of the record_value method. We have to provide a default implementation, because adding new methods to the Visit trait without defaults is a breaking change. If a visitor wishes to record Valuable values in a structured way, it should override the record_value method with an implementation that actually uses valuable to record that field.

For example, the JSON formatter in tracing-subscriber will do this when tracing-subscriber's valuable feature flag is enabled:

#[cfg(all(tracing_unstable, feature = "valuable"))]
fn record_value(&mut self, field: &Field, value: valuable_crate::Value<'_>) {
let value = match serde_json::to_value(valuable_serde::Serializable::new(value)) {
Ok(value) => value,
Err(_e) => {
#[cfg(debug_assertions)]
unreachable!(
"`valuable::Valuable` implementations should always serialize \
successfully, but an error occurred: {}",
_e,
);
#[cfg(not(debug_assertions))]
return;
}
};
self.values.insert(field.name(), value);
}

Hope that's helpful!

@marmistrz
Copy link

Thanks a lot!

@Yuhanun
Copy link

Yuhanun commented Apr 13, 2023

Is there any update on stabilization for valuable?

@cameronbraid
Copy link

Is there a way to get a Valuable to be formatted as json when using it as a span attribute ?

e.g.obj in the following

tracing::info_span!("MySpan", obj=request.as_value());

currently these are being formatted using the Debug impl

@jdnewman85
Copy link

jdnewman85 commented Feb 15, 2024

Is there a way to get a Valuable to be formatted as json when using it as a span attribute ?

e.g.obj in the following

tracing::info_span!("MySpan", obj=request.as_value());

currently these are being formatted using the Debug impl
Just ran into this, FYI

If you're using the json fmt subscriber, adding the valuable feature to tracing-subcriber is necessary.
Simply adding this dependency feature changed the same code from using Debug to being properly serialized as expected for the subscriber.

kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/core Related to the `tracing-core` crate crate/tracing Related to the `tracing` crate kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

No branches or pull requests

6 participants