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

Support custom timer via FormatTime #49

Closed
wants to merge 3 commits into from

Conversation

TmLev
Copy link
Contributor

@TmLev TmLev commented Jul 15, 2022

Resolves #48.

@TmLev TmLev mentioned this pull request Jul 15, 2022
Comment on lines +353 to +357
if let Some(timer) = self.timer.as_ref() {
timer
// TODO(TmLev): Wants `Writer`, have `event_buf`.
.format_time()
.expect("Unable to write time to buffer");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to actually call format_time() with event_buf since it requires &mut Writer which cannot be instantiated because of the private constructors:
https://github.com/tokio-rs/tracing/blob/8030eb9dfb56201b1b0843f6d6105eb89e3660ed/tracing-subscriber/src/fmt/format/mod.rs#L421-L431

Copy link
Owner

@davidbarsky davidbarsky Jul 16, 2022

Choose a reason for hiding this comment

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

Whoops, that's a bit of an issue. If the lack of a timer blocking you, let's not wait on tracing-subscriber. We can add a dependency on time directly and provide a few out-of-the-box formatters. I'm not sure if we should have our own FormatTime trait similar to that of tracing-subscrber's, but if we do, then I think we can probably tweak it so that it works well with tracing-tree (e.g., you can specify what writer you want to use here).

Getting tracing-tree onto tracing-subscriber's FormatEvent/FormatTime infrastructure might be a bigger lift that I don't want to commit to spending time on now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue about this in tracing repo: tokio-rs/tracing#2223. I suggest we wait for an answer from Eliza a little. If she agrees to make constructor public, then problem is solved (hopefully). If not, we can always mimic FormatTime for tracing-tree.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm aware of the issue you opened; check who the co-author of tracing-subscriber is :).

In any case, I think that a proper solution for this issue would be build atop of tokio-rs/tracing#2132 (but that issue is slightly orthogonal to this), but I'm pretty sure that we don't want to make a decision about the API in tracing-subscriber just yet. If we do, it'll require some larger changes across tracing-subscriber, and one of the best ways to determine what that API should be is to vendor the FormatTime trait in tracing-tree and experiment here. It has the nice side-effect of unblocking you as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for elaborating. I'll try to sketch an API similar to FormatTime from tracing-subscriber.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I hide FormatTime under the feature flag? Doing so complicates things with generic parameters and where clauses.

Copy link
Owner

Choose a reason for hiding this comment

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

it can be hidden behind a feature flag, but i wouldn't be too worried, honestly

@TmLev
Copy link
Contributor Author

TmLev commented Mar 28, 2023

Superseded by #56.

@TmLev TmLev closed this Mar 28, 2023
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.

Adding timestamps to output
2 participants