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

Value::from_serde ? #819

Open
anp opened this issue Jul 15, 2020 · 7 comments
Open

Value::from_serde ? #819

anp opened this issue Jul 15, 2020 · 7 comments
Labels
crate/core Related to the `tracing-core` crate kind/feature New feature or request kind/rfc A request for comments to discuss future changes

Comments

@anp
Copy link
Contributor

anp commented Jul 15, 2020

Feature Request

Motivation

I'd like to allow my subscriber to record arbitrarily nested objects from users' macros.

Proposal

I'd like some way to create a Value from the erased-serde traits (erased to reduce binary bloat). The basic sketch proposed by @KodrAus in rust-lang/log#328 (comment) for the log crate seems compelling to me but I haven't worked through all of the implications for tracing.

Alternatives

The most significant assumption of this request is that serde-based traits should be the public API. It's possible that we would be better off extending Value to support nesting in a different way (and offering a polyfill for serde implementors?).

It's also possible that Value is not the right place to include this compatibility but my naive impression is that it's probably the right call.

Another potential alternative would be to reuse the Value type from the log crate's kv support or to work with them to extract that type into a lower-level shared crate. This seems like a great target to drive towards but I suspect both projects will be better served by converging on a shared type once they both have their respective use cases understood and addressed.

@hawkw hawkw added kind/feature New feature or request crate/core Related to the `tracing-core` crate kind/rfc A request for comments to discuss future changes labels Jul 16, 2020
@hawkw
Copy link
Member

hawkw commented Jul 17, 2020

Related: #824 (which also deals with adding Value implementations for more structured types).

@hawkw
Copy link
Member

hawkw commented Jul 17, 2020

In general, while this sounds pretty simple, it's a pretty big issue, since it kind of depends on two tasks:

  • Make the Value trait user-implementable, and
  • Extend Value/Visit to support recording nested structures, like serde.

This is definitely something I think we all want to see --- currently, Value is sealed primarily to reserve the ability to make these changes without breaking existing uses! I just wanted to note what is required.

An ideal solution would meet the following goals:

  • zero-cost conversion/compatibility with serde/erased-serde...
  • ...without requiring tracing-core to depend on serde
  • have zero cost conversions/compatibility with log's structured value API
  • ...without requiring tracing-core to depend on log
  • be backwards-compatible with the current tracing Value system (e.g. not a breaking change)
  • require as little ceremony as possible for recording structured values

Some of these are more negotiable than others --- in particular, a feature-flagged serde dependency would be acceptable if absolutely necessary, since serde is 1.0.

Complete serde support would probably require tracing's Visit trait to include serde's entire data model. In the past, we opted to restrict the number of primitives significantly (we don't support floats, or anything smaller than u64, for example). It'd be worth getting @carllerche's take on whether or not the motivations for this are still relevant.

@KodrAus
Copy link
Collaborator

KodrAus commented Aug 4, 2020

I just merged in some refactoring to log's Value type that you might find interesting. @hawkw that includes the const-fn based specialization we checked out a while ago.

I've been wanting to pull the Value type out of log and turning it into a library for capturing some value in a structured bag. You mentioned you don't want tracing-core to depend on serde, which I think makes a lot of sense. Would you consider having tracing-core (possibly optionally) depend on this structured bag library that itself optionally depends on serde? So instead of having a series of methods on Visit to support nested values (I wrote a library for streaming nested values and there's a lot of design space there you may or may not want to take on) it has just one method for passing along one of these structured bags and the consumer can decide whether they want to treat it like a Serialize or something else.

@hawkw
Copy link
Member

hawkw commented Aug 7, 2020

I just merged in some refactoring to log's Value type that you might find interesting. @hawkw that includes the const-fn based specialization we checked out a while ago.

Very cool. I need to take a closer look at this strategy. Haven't had time yet, but I'm definitely interested in seeing what you've done. :)

@hawkw
Copy link
Member

hawkw commented Aug 7, 2020

Another possible solution for supporting serde (and pretty much anything else, it turns out), while requiring a pretty minimal change to the existing Value system, is what @davidbarsky suggested on issue
#845 (comment) (which was not really the right place for this suggestion, but I digress): make dyn Any + 'static a "primitive" value type:

A possible extension to this proposal (as well as #819, perhaps?) would be to allow tracing_core::Value to be implemented on T: dyn std::any::Any + 'static. This would allow layers to opt-in to handle formatting of arbitrary types without requiring crates such as hyperium/http to take a public dependency on tracing_core::Value. I haven't really thought through the ecosystem-wide implications of supporting this, but I suspect tracing-subscriber could support a registry of formatters for arbitrary types, such that subscribers and layers won't need to take an explicit dependency on a given formatter.

Then, we could add a Visit::record_any method to the Visit trait, where the visitor can just downcast the recorded type to whatever it wants (and, eventually, when we introduce ValueSet indexing, ValueSet::get_any). We would probably add a tracing::field::any helper function, and/or a new sigil, for recording something as an Any without having to cast it as a dyn Any.

This does have one significant downside: currently, all the record methods fall through to fmt::Debug. This means that if a subscriber does not have special handling for a particular type, it can just print it. However, an Any trait object is not known to implement fmt::Debug. This means that if you don't have a particular type to downcast to, you can't really do anything of value with an Any.

We could hack around that limitation by making the Any value type be T: Any + Debug, and recording it by wrapping it in a special struct that stores a dyn Any and a closure that downcasts the Any to its original type so that it can be formatted with Debug. But, this is a bit awkward.

This would also, critically, not allow us to record arbitrary Serialize types: only known ones. If the subscriber knows specific Any types it wants to serialize, it could downcast them and use their Serialize implementations. However, it cannot say "i will record anything that serde can serialize".

@davidbarsky
Copy link
Member

@hawkw Finally! I made a new issue: #905

@0xForerunner
Copy link

Don't know much about the details, but perhaps erased-serde could be useful here if object safety is the issue? This would be an amazing feature.

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 kind/feature New feature or request kind/rfc A request for comments to discuss future changes
Projects
None yet
Development

No branches or pull requests

5 participants