Skip to content
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

Include unfinished spans in transactions #1303

Closed
Tracked by #1690
bruno-garcia opened this issue Aug 31, 2021 · 3 comments · Fixed by #1592
Closed
Tracked by #1690

Include unfinished spans in transactions #1303

bruno-garcia opened this issue Aug 31, 2021 · 3 comments · Fixed by #1592

Comments

@bruno-garcia
Copy link
Member

bruno-garcia commented Aug 31, 2021

When a transaction is finished, the SDK tries to send the data to Sentry.
If an open Span (one that was not finished) exist in that transaction, the SDK removes that span before sending the data to Sentry.

That was done because Sentry will discard the transaction if unfinished spans are included. More context on this thread: getsentry/develop#274

An alternative solution used by the JavaScript SDK is to complete those spans with the timestamp of the finishing transaction and setting the status to timeline_exceeded. We should align with this approach as it shows something was going on even though it wasn't completed during the transaction.

Java: getsentry/sentry-java#1690
.NET: getsentry/sentry-dotnet#1182

@bruno-garcia
Copy link
Member Author

Reopening as the goal is to stop modifying the span status (or removing it for that matter) if a span is not finished.
Relay will take care of it: getsentry/relay#1244
Related .NET issue: getsentry/sentry-dotnet#1182

Worth noting once we revert the change here we'll need to notify the minimum Self Hosted version required since the old version of Relay dropped transactions completely if they had an unfinished span (reason why all SDKs compensated that with dropping or lying about a span being finished)

@philipphofmann
Copy link
Member

@bruno-garcia, wouldn't it be better to open another issue for reverting this change?

@philipphofmann
Copy link
Member

This was done with #1592.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants