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

core: Rework ValueSets to support indexing in addition to visiting #926

Open
davidbarsky opened this issue Aug 13, 2020 · 3 comments
Open
Labels
crate/core Related to the `tracing-core` crate meta/breaking This is a breaking change, and should wait until the next breaking release. needs/design Additional design discussion is required.

Comments

@davidbarsky
Copy link
Member

davidbarsky commented Aug 13, 2020

Feature Request

Crates

  • tracing-core

Motivation

The current tracing_core::field::ValueSet requires users to implement a visitor to process and record Values. While this is performant, users have expressed confusion and occasional frustration with this requirement.

Proposal

We should consider dispatching Value primitive values using an enum rather than a trait object in order to... (I'm not sure, @hawkw, can you add additional details?)

@davidbarsky davidbarsky added crate/core Related to the `tracing-core` crate needs/design Additional design discussion is required. meta/breaking This is a breaking change, and should wait until the next breaking release. labels Aug 13, 2020
@hawkw
Copy link
Member

hawkw commented Aug 13, 2020

@hawkw shared a sketch of a possible approach.

This is a sketch of the enum-based Value dispatch (#925), not of an indexable ValueSet.

@davidbarsky
Copy link
Member Author

Whoops, fixed. Sorry about that!

@hawkw
Copy link
Member

hawkw commented Aug 13, 2020

Okay, I talked it over with @carllerche a bit, and I think that the proposal for this is basically:

  • change ValueSet to be represented by &'a [Option<&'a dyn Value>] rather than &'a [(Field, Option<&'a dyn Value>)]
  • change the tracing-core ValueSet constructor to just require that the slice that's passed in is in order (and have the macros ensure that it is); document this on the off chance anyone else ever consumes this low-level API.
  • Higher level APIs can have some kind of builder for valuesets, or we can just deprecate/remove the record_all batched recording APIs. We could change the Subscriber::record method to just take a single field and Value --- AFAICT, nobody ever uses anything else.
  • Allow indexing ValueSets with Fields.

(note that Value will probably also stop being a trait object, but I didn't reflect that above because that's a separate issue, #925)

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 meta/breaking This is a breaking change, and should wait until the next breaking release. needs/design Additional design discussion is required.
Projects
None yet
Development

No branches or pull requests

2 participants