parity-common icon indicating copy to clipboard operation
parity-common copied to clipboard

Add support for valuable to fixed hashes

Open marmistrz opened this issue 2 years ago • 5 comments

Value::String was used for the visit implementation, because it's the most convienient format to use while tracing, AFAIK being the main motivation for the valuable crate.

marmistrz avatar May 16 '22 13:05 marmistrz

ping

marmistrz avatar May 23 '22 10:05 marmistrz

Hey @marmistrz. Could you clarify what you need this for exactly?

it's the most convienient format to use while tracing

what kind of tracing are you doing?

I'm hesitant to add yet another feature that will introduce unnecessary breaking changes to our crates unless it's really justified.

ordian avatar May 23 '22 10:05 ordian

This is not a breaking change: the trait implementation will only be used if the feature is enabled.

Tracing is a framework for instrumenting Rust programs to collect structured, event-based diagnostic information.

See https://tokio.rs/blog/2021-05-valuable

Without this change it's impossible to use #[derive(Valuable)] on any type (struct/enum) containing any parity primitive type.

marmistrz avatar May 23 '22 12:05 marmistrz

This is not a breaking change: the trait implementation will only be used if the feature is enabled.

I meant that when valuable version bumps to 0.2, updating fixed-hash to 0.2 would be a breaking change (unless they do it in a backwards compatible way for the current impl and we specify >=0.1 < 0.3 in Cargo.toml). So we're tying yet another dependency's breaking change to our crate.

There are requests to add other trait impls (e.g. #640) and at the moment, I'm trying to conservative and not add something unless it is really needed.

We could use a type like serde_json::Value to pass arbitrary structured data but this would require allocating and copying the data from our application's struct to hand it to the collector.

We already provide serde implementation if this Valuable is needed to avoid an allocation and copying the data in this case, I'm yet to be convinced we need it.

If there's consensus we do want to add support for valuable, it should be added to other crates like uint as well.

ordian avatar May 23 '22 13:05 ordian

I see. This implementation has to either be a part of valuable or primitive-types due to orphan rules. So far tracing doesn't support Value::from_serde.

marmistrz avatar May 24 '22 12:05 marmistrz