core: tracking issue for `valuable` integration

[hawkw]

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]

cc @carllerche

[davidbarsky]

(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.

[marmistrz]

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]

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:

tracing/tracing-subscriber/src/fmt/format/json.rs

Lines 481 to 499 in b9da544

	#[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]

Thanks a lot!