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 Configuration for Higher Time Precision with Decimals #68

Merged

Conversation

hrmsk66
Copy link

@hrmsk66 hrmsk66 commented Aug 28, 2023

To address #67, add a new configuration option, with_higher_precision. When this option is enabled, the time is displayed with higher precision, including decimal points.

  • Added with_higher_precision as a configuration option.
  • Refactored the existing code in the on_event method to separate methods for code reusability1

Example Output:
With higher_precision enabled, the output will look like this.

┐run{hostname="localhost"}
├─ 45.96μs TRACE Request received
├─┐fetch_httpbin{method=GET, path="/"}
│ ├─ 3.70s  DEBUG Response received, backend=httpbin, status=200 OK, cache_status=MISS
├─┘
├─┐fetch_example{method=GET, path="/"}
│ ├─ 587.93ms DEBUG Response received from example, backend=example, status=200 OK, cache_status=HIT
│ ├─┐open_datastore{name="subscriptions"}
│ ├─┘
│ ├─┐read_from_datastore{key="foo@example.com"}
│ │ ├─ 43.04μs  WARN Item found, key=foo@example.com
│ ├─┘
├─┘
├─ 4.29s  TRACE Returning response, status=200 OK
┘

Footnotes

  1. If this PR looks good, I'd like to propose an implementation to address Issue 17 based on this as a foundation.

Copy link
Collaborator

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just some style nits, then this lgtm

src/lib.rs Outdated
Comment on lines 352 to 509
let start = match ctx.current_span().id() {
Some(id) => match ctx.span(id) {
// if the event is in a span, get the span's starting point.
Some(ctx) => {
let ext = ctx.extensions();
let data = ext
.get::<Data>()
.expect("Data cannot be found in extensions");
Some(data.start)
}
None => None,
},
None => None,
};
if let Some(start) = start {
let elapsed = start.elapsed();
let millis = elapsed.as_millis();
let secs = elapsed.as_secs();
let (n, unit) = if millis < 1000 {
(millis as _, "ms")
} else if secs < 60 {
(secs, "s ")
} else {
(secs / 60, "m ")
};
let n = format!("{n:>3}");
write!(
&mut event_buf,
"{timestamp}{unit} ",
timestamp = self.styled(Style::new().dimmed(), n),
unit = self.styled(Style::new().dimmed(), unit),
)
.expect("Unable to write to buffer");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tip for the future: when moving things out into a new function, do that in a separate commit, and then modify it in other commits. This makes it easier to track for reviewers to see what changes you made in detail

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the advice. Will separate commits for clarity moving forward.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

lgtm, unfortunately you also need to rebase over the latest changes

Resolve merge conflicts in src/lib.rs and src/format.rs during rebase
…ion" changes.

Rebased to resolve merge conflicts and updated PR to match changes in commit 8ccafe8 by @ten3roberts. The commit simplifies how Span's Extensions is retrieved.
@hrmsk66 hrmsk66 force-pushed the hrmsk66/higher-precision-with-decimals branch from 59c8121 to 4c66dca Compare September 22, 2023 20:01
@hrmsk66
Copy link
Author

hrmsk66 commented Sep 22, 2023

Hi @oli-obk
Rebased to resolve conflicts and noticed my PR needed updates to align with the main branch. In the "concurrent span switching detection (8ccafe8)" commit by @ten3roberts, the steps for retrieving Span's Extensions seemed to have been simplified in the on_event method.

tracing-tree/src/lib.rs

Lines 476 to 487 in 8aa51bc

// check if this event occurred in the context of a span.
// if it has, get the start time of this span.
let start = match span {
Some(span) => {
// if the event is in a span, get the span's starting point.
let ext = span.extensions();
let data = ext
.get::<Data>()
.expect("Data cannot be found in extensions");
Some(data.start)
}

I've adjusted this PR accordingly in 4c66dca.

@oli-obk
Copy link
Collaborator

oli-obk commented Sep 27, 2023

Thanks!

@oli-obk oli-obk merged commit 5b5ebb6 into davidbarsky:main Sep 27, 2023
4 checks passed
@hrmsk66 hrmsk66 deleted the hrmsk66/higher-precision-with-decimals branch April 18, 2024 08:52
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