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

tracing-subscriber: refactor timings #3038

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

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Jul 23, 2024

Motivation

This makes the fmt::Subscriber code a bit simpler, and preps the
Timing type for lifting elsewhere (e.g. to be made public and part
of a timing layer instead of hidden within this Subscriber) (see
#2946)

Solution

This commit refactors the Timings struct to use Duration instead of
u64 to store the idle and busy times of a span, and moves the logic to
update the timings to the Timings struct itself. This commit also
introduces a display method to the Timings struct that returns the
idle and busy times as HumanReadableDuration instances (renamed from
TimingDisplay).

Replaces: #2944

This commit refactors the `Timings` struct to use `Duration` instead of
`u64` to store the idle and busy times of a span, and moves the logic to
update the timings to the `Timings` struct itself. This commit also
introduces a `display` method to the `Timings` struct that returns the
idle and busy times as `HumanReadableDuration` instances (renamed from
`TimingDisplay`).

Replaces: tokio-rs#2944
Copy link
Contributor

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

I like this more than #2944 👍

tracing-subscriber/src/fmt/fmt_subscriber.rs Show resolved Hide resolved
tracing-subscriber/src/fmt/fmt_subscriber.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Contributor Author

joshka commented Jul 23, 2024

What about renaming this to SpanTimings instead of Timings? It's currently private, so this isn't breaking.

Side note:
My personal style would be to move this to its own small module, as it's a neatly self contained piece that doesn't depend on other parts of the library. I like using modules to reduce the cognitive load of reading larger source files, but I don't know how this squares with the library maintainers, and everyone has different preferences on what sizes matter.

joshka added a commit to joshka/tracing that referenced this pull request Aug 17, 2024
This allows for the timing subscriber logic to be composed and used by
any subscriber rather than just users of the fmt subscriber.

TODO: work out how to properly configure the fmt subscriber to use the
timing subscriber as a layer.

Fixes: tokio-rs#2946
Replaces: tokio-rs#3038
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