-
Notifications
You must be signed in to change notification settings - Fork 5k
Record OperationCanceledException in HttpClient native instrumentation #116269
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl |
@antonfirsov this PR is ready to be reviewed. |
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 assuming tests are passing.
Not sure if it should be considered as a breaking (behavioral) change [...] For example, for .NET 10+, this logic will no longer be necessary
Given that folks typically depend on that OTel polyfill logic anyways and with this PR we are now doing mostly the same, IMO it is unlikely to break many people.
Failures in CI look unrelated. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #116205.
Not sure if it should be considered as a breaking (behavioral) change but with this change, when an
OperationCanceledException
occurs,Activity.Status
,error.type
tag, andexception
event will be set. For example, for .NET 10+, this logic will no longer be necessary: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/3df15a11fd77557d922f8e414914741e18ba1604/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L183-L193.