search

jonhoo / tracing-timing

Inter-event timing metrics on top of tracing.
113 stars 11 forks source link

Span close event #12

Closed akshayknarayan closed 3 years ago

akshayknarayan commented 3 years ago

tracing-subscriber has support for span-close events (https://docs.rs/tracing-subscriber/0.2.17/tracing_subscriber/fmt/struct.Layer.html#method.with_span_events), but these events aren't available to other layers.

For timing purposes, we already track the time from the start of the span, so this lets us track the time from the last event in a span to its close also. We can also get timing information about a span even if there are no events inside it.

codecov[bot] commented 3 years ago

Codecov Report

Merging #12 (f2aee57) into master (ddc0b9a) will decrease coverage by 0.26%. The diff coverage is 97.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   95.32%   95.05%   -0.27%     
==========================================
  Files           7        7              
  Lines        1027     1315     +288     
==========================================
+ Hits          979     1250     +271     
- Misses         48       65      +17
Impacted Files Coverage Δ
src/lib.rs 74.10% <ø> (-11.05%) :arrow_down:
src/layer.rs 86.00% <88.23%> (+0.70%) :arrow_up:
src/builder.rs 90.90% <100.00%> (+0.90%) :arrow_up:
src/subscriber.rs 75.75% <100.00%> (+1.60%) :arrow_up:
tests/layer.rs 100.00% <100.00%> (ø)
tests/subscriber.rs 100.00% <100.00%> (ø)
src/group.rs 100.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ddc0b9a...f2aee57. Read the comment docs.

akshayknarayan commented 3 years ago

Looks like minimum rust version might need to be increased.

jonhoo commented 3 years ago

I almost wonder if this should just be on by default — what do you think?

Looks like minimum rust version might need to be increased.

Go for it!

akshayknarayan commented 3 years ago

re: default, tracing_subscriber leaves it off by default: https://docs.rs/tracing-subscriber/0.2.17/tracing_subscriber/fmt/struct.Layer.html#method.with_span_events

I can see the argument that for this crate's purposes it should be on, though, since we're all about timing things (I definitely want this :), and I always set no_span_recursion). Maybe a v0.4.4 with this change, and a v0.5 changing the default?

jonhoo commented 3 years ago

I can see the argument that for this crate's purposes it should be on, though, since we're all about timing things (I definitely want this :), and I always set no_span_recursion). Maybe a v0.4.4 with this change, and a v0.5 changing the default?

Love it!

akshayknarayan commented 3 years ago

How are we doing on this? I think we are just waiting on feedback re: whether this is a terrible mistake?

jonhoo commented 3 years ago

Released as 0.4.4 :tada: