-
Notifications
You must be signed in to change notification settings - Fork 765
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
[AspNetCore] [Http] remove Activity Status Description and update unit tests #5025
[AspNetCore] [Http] remove Activity Status Description and update unit tests #5025
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5025 +/- ##
==========================================
- Coverage 83.60% 83.26% -0.35%
==========================================
Files 296 296
Lines 12452 12440 -12
==========================================
- Hits 10411 10358 -53
- Misses 2041 2082 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM - needs changelog.
For future:
Don't set the span status description if the reason can be inferred from http.response.status_code
Should get clarification from spec what happens when the response code is not available. Should we be setting the description in this case? If yes, then we can update it later(Post GA) (it won't be a breaking change).
Please do mention that EnrichWithException can be leveraged to get back the old behavior. This is important as this was the behavior for a very long time, so need to smoothen upgrade/migrate disruptions. |
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status The above section talks about guidelines on Description. Putting exception message may not meet this requirement (as exc. message could be unpredictable/not-easy-to-document as it is not a finite list?) |
@@ -2,6 +2,13 @@ | |||
|
|||
## Unreleased | |||
|
|||
* On Exception, Activity Status is set to Error. The Exception Message |
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.
Removed the Activity Status Description that was being set during unhandled exceptions. Activity Status will continue to be reported as Error
something like this would be more clear IMO
Towards #4983
Previously, these libraries were setting both the Activity Status & StatusDescription via
Activity.SetStatus(ActivityStatusCode, Description)
. This PR removes thestring
description.The Http spec states:
Changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes