Uh oh!
There was an error while loading. Please reload this page.
Expose end_timestamp in Span#finish and Transaction#finish - #1946
Conversation
codecov-commenter
commented
Nov 23, 2022
Codecov ReportBase: 96.56% // Head: 98.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@## neel/otel/expose-span-id #1946 +/- ##
============================================================
+ Coverage 96.56% 98.35% +1.78%
============================================================
Files 121 151 +30 Lines 7723 9380 +1657 ============================================================
+ Hits 7458 9226 +1768 + Misses 265 154 -111
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
st0012
left a comment
There was a problem hiding this comment.
I think this can base on master instead.
| def finish | ||
| def finish(end_timestamp: nil) | ||
| # already finished | ||
| return if @timestamp |
There was a problem hiding this comment.
This isn't mentioned in the spec so I want to double check it: we only allow the first override but not the second time or later?
If that's true, can we add a test case for it?
There was a problem hiding this comment.
ah no, it should override either way, good catch!
| @@ -90,11 +90,11 @@ def initialize( | |||
| # Finishes the span by adding a timestamp. | |||
| # @return [self] | |||
There was a problem hiding this comment.
We forgot to mention that it could return nil too (early return)
There was a problem hiding this comment.
I think it should never return nil, will also fix
f2d6d71 to
e863e25Comparest0012
left a comment
There was a problem hiding this comment.
I think we can merge this once the changelog entry is updated + the build is fixed by updating the base branch.
954d6a2 to
833392fComparee863e25 to
85d1c67Compare
No description provided.