core: update ValueSet and FieldSet to use Valuable

[bryangarza]

This is a WIP draft.

Depends on: https://github.com/tokio-rs/valuable/pull/99

What is missing:

Dealing with

error[E0277]: `&dyn valuable::Structable` is not an iterator
   --> tracing-core/src/field.rs:871:31
    |
871 |         for (field, value) in self.values {
    |                               ^^^^^^^^^^^ `&dyn valuable::Structable` is not an iterator
    |
    = help: the trait `Iterator` is not implemented for `&dyn valuable::Structable`
    = note: required because of the requirements on the impl of `IntoIterator` for `&dyn valuable::Structable`

[hawkw]

thinking about this a bit more --- i mentioned in a few places that we might want to completely remove the FieldSet and ValueSet types, e.g.:

I think we can probably get rid of the FieldSet type entirely, and replace it with the valuable::StructDef defined by the span/event's Structable impl?

but, on the other hand, it occurs to me that maybe we should keep these around, if we want to decou0ple tracing-core's stability story from valuable's. if we kept our own types, we could have something like:

struct FieldSet<'a> {
     inner: StructDef<'a>,
}

impl FieldSet<'_> {
     pub const fn from_valuable_01(inner: valuable::StructDef<'_>) -> Self {
          Self { inner }
     }

     // ...
}

that way, if we wanted to add support for a future breaking release of valuable without breaking tracing-core, we could potentially change the internal representation of FieldSet to an enum of the valuable 0.1 and valuable 0.2 types, and add a from_valuable_02 constructor. if we're careful, a scheme like that could potentially allow us to release a breaking tracing release to pick up a new valuable, without having to break tracing-core --- we could have a hypothetical tracing 0.3, and tracing-core 0.2 could potentially support both tracing 0.3 and tracing 0.2...

@carllerche, what do you think? is something like this worth the effort, or should we just tightly couple tracing-core and valuable's versioning?

[bryangarza]

Thanks for your comments @hawkw! I do think it'd be great to incrementally merge changes into another branch, otherwise the PR is going to take forever. I created the valuable branch now, and pointed the PR to it.

[hawkw]

Thanks for your comments @hawkw! I do think it'd be great to incrementally merge changes into another branch, otherwise the PR is going to take forever. I created the valuable branch now, and pointed the PR to it.

great, sounds good to me!

[bryangarza]

thinking about this a bit more --- i mentioned in a few places that we might want to completely remove the FieldSet and ValueSet types, e.g.:

I think we can probably get rid of the FieldSet type entirely, and replace it with the valuable::StructDef defined by the span/event's Structable impl?

but, on the other hand, it occurs to me that maybe we should keep these around, if we want to decou0ple tracing-core's stability story from valuable's.

It seems useful to have a really thin wrapper around valuable, if we want to guarantee that tracing-core's API will remain stable. If we did that, how would we handle exposing valuable's API in a backwards-compatible way? Like, let's say that valuable releases a breaking change on the Visit trait -- how would we deal with that in pub fn record(&self, visitor: &mut dyn Visit)? Is it possible to accept some enum here as well depending on what version of valuable is being pulled in? Is it inevitable that we make a breaking release to tracing-core in that case? If a wrapper only gets us part of the way there, it might not be worth it. I'm curious how other libraries are handling this problem. @hawkw

[hawkw]

@bryangarza

Like, let's say that valuable releases a breaking change on the Visit trait -- how would we deal with that in pub fn record(&self, visitor: &mut dyn Visit)?

I don't think we would want to have a record method taking a &mut dyn Visit, if we went with this approach. Instead, we would simply implement the Valuable trait for our ValueSet type. Then, when a new version of valuable is released, we would depend on both valuable 0.1 and valuable 0.2, and our ValueSet type would implement both valuable_01::Valuable and valuable_02::Valuable.

The record method isn't really necessary, as long as our types implement Valuable, they can be recorded by any implementation of valuable::Visit.

[bryangarza]

Oh okay, that makes mores sense

[bryangarza]

Okay, I think the rest of the upcoming changes (namely, deleting/replacing FieldSet, and deleting some of the now-unneeded code) and updating the rest of tracing-core to match) can be done in a separate PR into the valuable branch.

[carllerche]

@hawkw Valuable pretty much exists to support tracing-core. We will track breaking change releases. I would not jump through too many hoops to hide valuable.