-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Avoid unnecessary time.Since() when ending spans if the endTime is already specified #3143
base: main
Are you sure you want to change the base?
Avoid unnecessary time.Since() when ending spans if the endTime is already specified #3143
Conversation
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3143 +/- ##
=======================================
- Coverage 79.7% 79.7% -0.1%
=======================================
Files 171 171
Lines 12673 12674 +1
=======================================
- Hits 10105 10104 -1
- Misses 2355 2357 +2
Partials 213 213
|
@@ -347,17 +347,24 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) { | |||
return | |||
} | |||
|
|||
config := trace.NewSpanEndConfig(options...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other impact can moving the processing of config options before the IsRecording()
short-circuit have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Aneurysm9, we only have two kinds of SpanEndOption
, which are trace.WithStackTrace
and trace.WithTimestamp
. And the config := trace.NewSpanEndConfig(options...)
is allocating on the stack, no GC impact. Therefore the impact will be looping at most two iterations on the options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone understand the comment below this, "now that we have an end time and see if we need to do any more processing."
I can't see why we'd compute the timestamp before the short-circuit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone understand the comment below this, "now that we have an end time and see if we need to do any more processing."
I can't see why we'd compute the timestamp before the short-circuit.
Because if we didn't the end time would be less accurate.
I'm not sure if this is still an issue that is wanting to be done, but I did a small benchmark and got these results with this change (with a much more recent branch)
This is the benchmark I used https://go.dev/play/p/--mM4hpzckk Is this something we want to try to proceed with for some small savings? |
Hi,
When ending a span, I noticed that there might be an unnecessary and expensive
time.Since
invocation (inside theinternal.MonotonicEndTime
) that we can avoid:Saying that I want to instrument the trace and the latency metric at the same time, I would like to make sure I use only two expensive
time.Now
invocations:So, if there is a
trace.WithTimestamp(ts)
passed in while ending the span, we don't need theinternal.MonotonicEndTime
invocation.To do that, this PR moves
config := trace.NewSpanEndConfig(options...)
forward and check if there is atrace.WithTimestamp(ts)
passed in and callinternal.MonotonicEndTime
only when necessary.