Set error status if http has exception and ok status. #1143
Conversation
|
Does this happen because the status code is set after the middleware? |
yup |
| else if (status == SpanStatus.Ok) | ||
| { | ||
| transaction.Finish(exception); |
There was a problem hiding this comment.
| else if (status == SpanStatus.Ok) | |
| { | |
| transaction.Finish(exception); | |
| // Status code not yet changed to 500 but an exception does exist | |
| // so lets avoid passing the misleading 200 down and close only with | |
| // the exception instance that will be inferred as errored | |
| else if (status == SpanStatus.Ok && exception is not null) | |
| { | |
| transaction.Finish(exception); |
There was a problem hiding this comment.
Doesn't the previous lines guarantee that you don't have a null exception?
if (exception is null)
{
transaction.Finish(status);
}
else if (status == SpanStatus.Ok)
{
transaction.Finish(exception);
}
else
{
transaction.Finish(exception, status);
}There was a problem hiding this comment.
Slightly edited the test Transaction_binds_exception_thrown for checking if the Span was set as Internal Error.
Without the changes in this PR, the test was returning a span with the Status Ok.
There was a problem hiding this comment.
Yeah it already checks for null
There was a problem hiding this comment.
OK, please add the comments
Codecov Report
@@ Coverage Diff @@
## main #1143 +/- ##
==========================================
- Coverage 80.58% 80.57% -0.01%
==========================================
Files 203 203
Lines 6675 6677 +2
Branches 1477 1478 +1
==========================================
+ Hits 5379 5380 +1
- Misses 816 817 +1
Partials 480 480
Continue to review full report at Codecov.
|
…tps://github.com/getsentry/sentry-dotnet into ref/internal-error-on-transaction-with-exception
| else if (status == SpanStatus.Ok) | ||
| { | ||
| transaction.Finish(exception); |
There was a problem hiding this comment.
OK, please add the comments
| @@ -1,4 +1,4 @@ | |||
| #if !NETCOREAPP2_1 | |||
| #if !NETCOREAPP2_1 | |||
There was a problem hiding this comment.
not sure what has changed, at least, vs code doesn't spot it https://github1s.com/getsentry/sentry-dotnet/pull/1143
bruno-garcia
deleted the
ref/internal-error-on-transaction-with-exception
branch
InvokeAsync middleware doesn't receive the correct status code during an error, setting a transaction with an error as "OK", this PR sets the default status code based on the received Exception and if the status code is Ok.
Before
After
