Skip to content

Conversation

sunng87
Copy link
Contributor

@sunng87 sunng87 commented Jul 19, 2021

This patch provides an implementation of Valuable for serde_json::Value. I'm opening this as a demo of implementing Valuable on a complex data type. This is better to be placed in serde_json crate when valuable released on crates.io. So it's not necessarily to be merged.

The reason why it is implemented in valueable crate is because an external one requires wrapper type over serde_json::Value. Furthermore, for collection types it may bring in additional allocation.

The second part is about the choice between Mappable and Structable for serde_json::Map. Using Mappable is straight-forward but it's lack of key access. Creating StructDef from the map might not be possible because of the slice lifetime of Fields::Named. The is probably we need to consider during the API design.

@sunng87 sunng87 force-pushed the feature/valuable-json branch from 8d44650 to 2ccc2bf Compare July 19, 2021 15:37
@Vrajs16
Copy link

Vrajs16 commented Apr 25, 2025

Is there any remaning work that needs to be done for this too be merged. Will be really helpful for structured logging in tracing

@michaeltlombardi
Copy link

If there's any remaining work or concerns for this PR, I would be happy to tackle it. A project I'm currently working on has a need for consuming arbitrary trace data from external programs and we're already using JSON Lines for emitting messages that our program can bubble up. Unfortunately, because we can't implement Valuable on serde_json::Value ourselves, we would need to use the newtype pattern to wrap the incoming data (and also unwrap it if we add support for more known structured data fields so we can validate them correctly).

It makes sense to me for this crate to implement Valuable on serde_json::Value given its ubiquity. I would also be open to adopting the implementation in serde_json (see: serde-rs/json#1255) if the preference of the maintainers is to not accept an implementation in this crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants