-
Notifications
You must be signed in to change notification settings - Fork 220
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
fix(otel): Overwrite "trace" context in Sentry error #580
Conversation
Codecov ReportBase: 79.08% // Head: 78.98% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #580 +/- ##
==========================================
- Coverage 79.08% 78.98% -0.10%
==========================================
Files 38 38
Lines 3859 3860 +1
==========================================
- Hits 3052 3049 -3
- Misses 705 708 +3
- Partials 102 103 +1
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. |
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.
https://github.com/getsentry/sentry-go/blob/master/scope.go#L359-L374
Let's remove the logic in StartSpan
and set the trace
context in scope.ApplyToEvent()
instead.
@cleptric Not entirely sure what you're suggesting, but it sounds larger/riskier than what was initially planned for this PR. Can you open an issue and explain your idea there please? |
This patches the following issue: when creating a transaction in the span processor, we later overwrite its SpanID/TraceID. The problem is, the trace context is also set on the scope when the span is started:
sentry-go/tracing.go
Line 176 in f03b31a
...which leads to broken error-transaction linking.
So ultimately we need to either/or:
Here I'm taking approach 2 because it's easier, but generally speaking I believe we should review setting trace context (and a few other tracing related things) on the scope.