-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Set error status if http has exception and ok status. #1143
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 |
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.
I think we need to check if we have an instance because passing null
as exception could behave as unknown
status or as ok
or that can change in the future.
There are no tests for this so sounds like we can change this and nothing validates the change.
else if (status == SpanStatus.Ok) | ||
{ | ||
transaction.Finish(exception); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it already checks for null
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, please add the comments
@@ -1,4 +1,4 @@ | |||
#if !NETCOREAPP2_1 | |||
#if !NETCOREAPP2_1 |
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.
BOM?
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.
not sure what has changed, at least, vs code doesn't spot it https://github1s.com/getsentry/sentry-dotnet/pull/1143
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