Finish unfinished Spans on Transaction completion #1296

lucas-zimerman · 2021-11-02T20:09:12Z

Also, consider as Errored Events with Level greater or equal to Error.

src/Sentry/SentryEventExtensions.cs

bruno-garcia · 2021-11-03T19:53:17Z

src/Sentry/SentryEventExtensions.cs

+            => @event.Level >= SentryLevel.Error ||
+               @event.Exception is not null ||
+               @event.SentryExceptions?.Any() == true;


Is there an IntenralExceptions instead? Otherwise it'll instantiate a list on the property getter IIRC

Suggested change

=> @event.Level >= SentryLevel.Error ||

@event.Exception is not null ||

@event.SentryExceptions?.Any() == true;

=> @event.Level >= SentryLevel.Error

|| @event.Exception is not null

|| @event.InternalSentryExceptions?.Any() == true;

There's no InternalSentryExceptions.

We could use @event.HasException but that's basically the same code

sentry-dotnet/src/Sentry/SentryEvent.cs

Line 167 in 997a310

internal bool HasException => Exception is not null || SentryExceptions?.Any() == true;

sentry-dotnet/src/Sentry/SentryEvent.cs

Lines 75 to 84 in 41a9ab9

internal SentryValues<SentryException>? SentryExceptionValues { get; set; }

/// <summary>

/// The Sentry Exception interface.

/// </summary>

public IEnumerable<SentryException>? SentryExceptions

{

get => SentryExceptionValues?.Values ?? Enumerable.Empty<SentryException>();

set => SentryExceptionValues = value != null ? new SentryValues<SentryException>(value) : null;

}

test/Sentry.Tests/HubTests.cs

bruno-garcia · 2021-11-03T19:55:47Z

test/Sentry.Tests/HubTests.cs

+            Assert.Equal(_fixture.TraceId, @event.Contexts.Trace.TraceId);
+            Assert.Equal(_fixture.ParentId, @event.Contexts.Trace.ParentSpanId);
+            Assert.True(span.IsFinished);
+            Assert.Equal(SpanStatus.UnknownError, span.Status);


This makes me realize that we're diverging here. Other SDKs are defaulting to Ok when no status is set. @marandaneto recently talked about this on another repo. He might be able to chime in here

I have to agree that it was the expected result that I was expecting by calling Finish() but instead I got UnknownError.

I could make a separate PR to alter the default Finish status and then update this PR.

for HTTP spans -> getsentry/develop#307 (comment)

if its a UI transaction, default to ok unless there's an exception (then INTERNAL_ERROR).

if there's no status at all, relay (ingestion) or UI not sure defaults to unknown

test/Sentry.Tests/HubTests.cs

src/Sentry/Internal/Hub.cs

…ed Spans.

lucas-zimerman · 2021-11-04T19:49:34Z

I'll fix the DiagnosticSource integration for SQLClient on another PR since once merged this PR will make the SQL Client integration generate some noise (for instance, multiple connections spans for the same connection id).

src/Sentry/Transaction.cs

codecov-commenter · 2021-11-04T20:26:50Z

Codecov Report

Merging #1296 (bb0063e) into main (78aa050) will decrease coverage by 0.02%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #1296      +/-   ##
==========================================
- Coverage   82.60%   82.58%   -0.03%     
==========================================
  Files         213      213              
  Lines        7116     7119       +3     
  Branches     1401     1402       +1     
==========================================
+ Hits         5878     5879       +1     
- Misses        803      807       +4     
+ Partials      435      433       -2

Impacted Files	Coverage Δ
src/Sentry/Internal/Hub.cs	`73.26% <ø> (-0.29%)`	⬇️
src/Sentry/SentryEventExtensions.cs	`0.00% <0.00%> (ø)`
src/Sentry/Transaction.cs	`82.55% <100.00%> (ø)`
src/Sentry/TransactionTracer.cs	`94.44% <100.00%> (+0.19%)`	⬆️
src/Sentry/ScopeExtensions.cs	`70.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78aa050...bb0063e. Read the comment docs.

…thub.com/getsentry/sentry-dotnet into fix/message-only-errors-not-closing-span

bruno-garcia · 2021-11-05T22:43:40Z

test/Sentry.Tests/HubTests.cs

+        [Theory]
+        [InlineData(SentryLevel.Error)]
+        [InlineData(SentryLevel.Fatal)]
+        public void CaptureEvent_ErroredMessageWithoutException_ClosesCurrentSpanAsInternalError(SentryLevel level)


I'm not sure we should close the span. This will affect the end timestamp by setting the end of the span to the time the error happened, even if the span was still running for a long time after.

Capturing the even should include the trace information, which will help Sentry show in the span that there was a related error. The time the span finishes shouldn't be affected. I believe neither should the status. Say there was a try/catch. The user logged the error, but handled it, and had a retry logic that then succeeded. The overall span was OK, it took longer because it did more things than it needed if it had succeeded in the first place though.

Since spans are getting closed once the Transaction is concluded, I have removed the special code that was closing the spans if none of them were sampled.

…thub.com/getsentry/sentry-dotnet into fix/message-only-errors-not-closing-span

SimonCropp · 2021-11-23T21:05:35Z

so this was a breaking change since it removed public static class SentryEventExtensions

add test case.

c2d186b

lucas-zimerman added the Bug Something isn't working label Nov 2, 2021

lucas-zimerman and others added 5 commits November 2, 2021 17:14

Update test.

872c2c1

removed some moqs

e2a32d1

Code cleanup

5591695

Close spans with events without exceptions.

34c498a

Format code

ae96241

bruno-garcia reviewed Nov 3, 2021

View reviewed changes

lucas-zimerman added 2 commits November 4, 2021 14:47

rollback Hub CaptureEvent, removed A and implemented close unfinish…

cfb2519

…ed Spans.

typo.

9d3ddeb

lucas-zimerman changed the title ~~Fix Session not being closed on errored event without exception (DONT MERGE)~~ Finish unfinished Spans on Transaction completion (DONT MERGE) Nov 4, 2021

lucas-zimerman added 2 commits November 4, 2021 16:38

Tests fixes.

d04bf92

Merge branch 'main' into fix/message-only-errors-not-closing-span

8038daa

lucas-zimerman commented Nov 4, 2021

View reviewed changes

src/Sentry/Transaction.cs Show resolved Hide resolved

lucas-zimerman changed the title ~~Finish unfinished Spans on Transaction completion (DONT MERGE)~~ Finish unfinished Spans on Transaction completion Nov 4, 2021

lucas-zimerman added 3 commits November 5, 2021 11:21

small refactor

657c9b5

Merge branch 'fix/message-only-errors-not-closing-span' of https://gi…

3c943fd

…thub.com/getsentry/sentry-dotnet into fix/message-only-errors-not-closing-span

Merge branch 'main' into fix/message-only-errors-not-closing-span

0bd5097

lucas-zimerman marked this pull request as ready for review November 5, 2021 14:23

lucas-zimerman added 2 commits November 5, 2021 11:25

Update changelog.

272eece

Self code review fixes.

07374b2

bruno-garcia requested changes Nov 5, 2021

View reviewed changes

lucas-zimerman and others added 5 commits November 5, 2021 20:23

Moved OR condition to the left side.

f30239b

Removed unneeded Span close.

a92e53c

Merge branch 'main' into fix/message-only-errors-not-closing-span

1223d3d

Format code

dc2895a

Merge branch 'main' into fix/message-only-errors-not-closing-span

bb0063e

lucas-zimerman requested a review from bruno-garcia November 8, 2021 18:34

lucas-zimerman added 2 commits November 8, 2021 15:41

Removed unused extension.

fa2aa9e

Merge branch 'fix/message-only-errors-not-closing-span' of https://gi…

b25266a

…thub.com/getsentry/sentry-dotnet into fix/message-only-errors-not-closing-span

bruno-garcia approved these changes Nov 8, 2021

View reviewed changes

bruno-garcia merged commit b8aee28 into main Nov 8, 2021

bruno-garcia deleted the fix/message-only-errors-not-closing-span branch November 8, 2021 20:52

lucas-zimerman mentioned this pull request Nov 8, 2021

Fix: Don't leave leftovers for SQLClient #1317

Merged

SimonCropp mentioned this pull request Nov 23, 2021

Dont autoverify on BuildServer #1341

Merged

jamescrosswell mentioned this pull request Jan 5, 2024

Include unfinished spans in transactions #1182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish unfinished Spans on Transaction completion #1296

Finish unfinished Spans on Transaction completion #1296

lucas-zimerman commented Nov 2, 2021 •

edited

Loading

bruno-garcia Nov 3, 2021

lucas-zimerman Nov 4, 2021

bruno-garcia Nov 5, 2021

bruno-garcia Nov 3, 2021

lucas-zimerman Nov 4, 2021

marandaneto Nov 5, 2021 •

edited

Loading

marandaneto Nov 5, 2021

lucas-zimerman commented Nov 4, 2021

codecov-commenter commented Nov 4, 2021 •

edited

Loading

bruno-garcia Nov 5, 2021

lucas-zimerman Nov 8, 2021

SimonCropp commented Nov 23, 2021

	internal SentryValues<SentryException>? SentryExceptionValues { get; set; }

	/// <summary>
	/// The Sentry Exception interface.
	/// </summary>
	public IEnumerable<SentryException>? SentryExceptions
	{
	get => SentryExceptionValues?.Values ?? Enumerable.Empty<SentryException>();
	set => SentryExceptionValues = value != null ? new SentryValues<SentryException>(value) : null;
	}

Finish unfinished Spans on Transaction completion #1296

Finish unfinished Spans on Transaction completion #1296

Conversation

lucas-zimerman commented Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-zimerman commented Nov 4, 2021

codecov-commenter commented Nov 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonCropp commented Nov 23, 2021

lucas-zimerman commented Nov 2, 2021 •

edited

Loading

marandaneto Nov 5, 2021 •

edited

Loading

codecov-commenter commented Nov 4, 2021 •

edited

Loading