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

Add support for valuable to fixed hashes #642

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marmistrz
Copy link

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
Copy link
Author

ping

@ordian
Copy link
Member

ordian commented May 23, 2022

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.

@marmistrz
Copy link
Author

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.

@ordian
Copy link
Member

ordian commented May 23, 2022

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.

@marmistrz
Copy link
Author

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.

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.

None yet

2 participants