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

[wip] draft API for passing timer into formatters #2132

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented May 20, 2022

This is a rough draft of an API for passing timestamps into any formatter, rather than just ones that use fmt::Format. I haven't done all of the plumbing for this change yet, just sketched out the APIs.

It's not clear if we can do this without a breaking change, unfortunately, since the with_timer method changes a type parameter of fmt::Format. We would want to change with_timer to not require that the event formatter be fmt::Format...but if we did that, we would no longer be able to change its type parameter.

One non-breaking option is to leave that system in place but deprecate it in favor of a new API that works with any event formatter, but then we would have to come up with a better name...

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from davidbarsky and a team as code owners May 20, 2022 16:45
hawkw added 3 commits May 20, 2022 10:18
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Comment on lines 71 to 72
// XXX(eliza): could make this a generic param as a breaking change...
timer: Option<Box<dyn time::FormatTime + Send + Sync>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentioned this previously, but we can add a new generic parameter so long that we provide a default for the generic. I'm guessing some form of UTC time is probably best.

hawkw and others added 3 commits May 20, 2022 12:53
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@davidbarsky
Copy link
Member

This is a pretty helpful change, thanks. Upon working more with tracing-glog, would it be a good idea to expose some of the simpler bits of configuration (e.g., thread names, files, targets) available on Format to the subscriber as a FmtCfg struct, or should I should just bite the bullet and make tracing-glog a layer instead of a formatter atop of fmt?

(I suppose this impacts the logfmt formatter you were playing around with as well.)

@hawkw
Copy link
Member Author

hawkw commented Jun 6, 2022

Upon working more with tracing-glog, would it be a good idea to expose some of the simpler bits of configuration (e.g., thread names, files, targets) available on Format to the subscriber as a FmtCfg struct, or should I should just bite the bullet and make tracing-glog a layer instead of a formatter atop of fmt?

unfortunately, with the way that struct currently works, this is a bit annoying because the Format struct wraps a marker type that determines what formatting is used, rather than belonging to a formatter type. This means it can't easily be used in external crates, because an impl Format<MyType> is an implementation for a foreign type.

i think we might want to redesign this part of the formatter system so that a config struct can be passed in to formatters from other crates, instead, but that seems like a separate change...

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.

2 participants