Feature/valuable integration

[xd009642]

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:

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=value)
}

fn blahblah(person: &Person) -> tracing::Span {
    info_span!("span", v=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);
    blahblah(&person);
}

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:

error[E0277]: the trait bound `Person: tracing::Value` is not satisfied
  --> src/main.rs:23:5
   |
23 |     info_span!("span", v=person)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `tracing::Value` is not implemented for `Person`
   |
   = note: required because of the requirements on the impl of `tracing::Value` for `&Person`
   = note: required for the cast to the object type `dyn tracing::Value`
   = note: this error originates in the macro `$crate::valueset` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Person: tracing::Value` is not satisfied
  --> src/main.rs:41:5
   |
41 |     info_span!("Greeting", person=person);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `tracing::Value` is not implemented for `Person`
   |
   = note: required for the cast to the object type `dyn tracing::Value`
   = note: this error originates in the macro `$crate::valueset` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `tracing-valuable-test` due to 2 previous errors

[xd009642]

Okay after some useful guidance from @hawkw on discord I've added a ValuableValue type (candidates for better names more than welcome). And the following minimal example compiles.

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 RUSTFLAGS="--cfg tracing_unstable" as suggested in the tracking issue by @davidbarsky. I guess my only question for that would be should it still be an optional feature then or remove the feature entirely in favour of the cfg. Either way that should be a simple change so I figured i'd do that as part of the tidy up once everyone's happy with how it's currently looking and working

[hawkw]

I guess my only question for that would be should it still be an optional feature then or remove the feature entirely in favour of the cfg.

Regardless of whether or not we add an unstable cfg, it should be an optional dependency. The goal is that the tracing_unstable cfg would be temporary, and it would eventually be removed. However, once the unstable cfg is removed, I think we would still want users to be able to determine whether or not the valuable dependency is enabled. So, if we add the cfg, we should still feature flag as well.

[hawkw]

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 valuable. Since we can't publish new tracing releases if we have git dependencies, merging this PR would block publishing tracing releases until valuable is on crates.io.

Therefore, since this is currently blocked on a published valuable release, I'm marking this PR as a draft, since it can't be merged yet for reasons not directly related to the code change here.

cc @carllerche

[xd009642]

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 😄

[hawkw]

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!

[hawkw]

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.

Suggested change

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

[xd009642]

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

[hawkw]

Another good way to test out that this actually works correctly would be writing a few quick examples in the examples/ dir. That way, we can demonstrate that it works, and when it's ready to merge, we'll already have some nice examples for users to learn how to use it.

[xd009642]

Added a very simple example, emphasis on very simple it just derives Valuable on a type adds it as a field to a span and then emits an event with the default format subscriber so you can see it printed out. I also cribbed the person/address example from valuable again with a lord of the rings reference because it was the first thing to come to mind that shouldn't covered by copyright wasn't my own name and address 😅

Clippy doesn't like the example without a main function though, so maybe attributes on everything is a nicer approach just for CI

[hawkw]

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 unstable-examples and put all the examples that require tracing-unstable (currently only the valuable ones) in that crate. then, we could build the unstable examples crate separately, and have it require the unstable feature to be enabled?

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 main seems to actually be a compile error. so, we can't just slap an #[allow(clippy::no_main)] in that file, or something like that; the missing main function is a hard error, not an ignorable warning. :(

[hawkw]

I just realized that this branch is based on master rather than v0.1.x, but this change is the v0.1 version of valuable support, so it should really be based on the v0.1.x branch.

I tried changing the base in the github UI, but this branch also contains a bunch of unrelated changes from master. @xd009642, would you mind rebasing this onto v0.1.x, dropping all the commits from master? It should hopefully rebase cleanly; you may want to use git rebase -i and drop all the commits that aren't part of the valuable integration change.

[bridiver]

this causes cargo to download the valuable crate even though it isn't actually used without tracing_unstable