Fix intermittent errors in trace spans comparisons #105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grcevski
changed the title
Codecov Report
@@ Coverage Diff @@
## main #105 +/- ##
===========================================
- Coverage 73.99% 47.76% -26.23%
===========================================
Files 20 19 -1
Lines 1496 1411 -85
===========================================
- Hits 1107 674 -433
- Misses 293 683 +390
+ Partials 96 54 -42
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mariomac
approved these changes
May 8, 2023
pkg/transform/spanner.go
Outdated
| return s.RequestStart >= parent.RequestStart && s.End <= parent.End | ||
| } | ||
|
|
||
| func (s *HTTPRequestSpan) Timings() (time.Time, time.Time, time.Time) { |
There was a problem hiding this comment.
for better API readability, I'd maybe use named return values in the signature:
func (s *HTTPRequestSpan) Timings() (goroutineStart time.Time, requestStart time.Time, end time.Time)Or even returning a structure:
type Timings struct {
GoroutineStart time.Time
RequestStart time.Time
End time.Time
}There was a problem hiding this comment.
Great suggestion! I'll fix it in this PR.
|
Thanks Mario! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up to #104.
For the purpose of nested spans we compare where the client spans land to find the matching server span. The earlier approach used the wall-clock converted times, which can be unstable and in some local stress testing I noticed that sometimes the span nesting doesn't work properly.
I've changed the HTTPRequestSpan such that it contains the original mono time captured by the bpf probes, and we only convert to wall-clock time when we need to make the trace or metric span. This should also help with performance and memory consumption. Namely size metrics will never have to do any conversion on timestamps and we'll keep lower memory footprint on the span channel.
I also found a bug while investigating this issue, I had put the HTTP client probes on the Gin registration too, so they were firing double. The required modules should notice we need the HTTP eBPF program too.