-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Replace http.error.reason with OTel standard error.type #91910
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAddress #91909. This PR should be only merged if we its' 8.0 servicing counterpart will be also approved, which I plan to open soon.
|
Is there any similar change needed in Kestrel @JamesNK |
No aspnetcore changes are required. This attribute is for HTTP clients only. |
Ok. Yes sounds like we should definitely get into 8.0 as suggested |
Let's get this into 9.0 first, then we can immediately create backport to 8.0. IMO it should not have a problem to get in. @artl93 heads up. If you have concerns, please let us know ASAP. |
@MihaZupan @noahfalk can you please code review, so that we can initiate the backport? |
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.
Looks good, thank you
src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs
Outdated
Show resolved
Hide resolved
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.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Looks great 👍
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6172076292 |
Fixes #91909
Align attribute name and semantics with the OpenTelemetry spec (which was finalized yesterday 2023/9/11 - see open-telemetry/semantic-conventions#205) --
http.error.reason
attribute has been standardized under nameerror.type
. We need to:http.error.reason
toerror.type
.error.type
also for valid responses where the HTTP status code indicates an error (4xx or 5xx) -- in such case, the value should be the string representation of the HTTP status code.http.error.reason
only when the underlying handler fails to fetch a response.See
error.type
in the spec for more details.For context: We introduced the attribute
http.error.reason
in PR #89809 (on 2023/8/3) based on the in-progress draft of the OTel spec (open-telemetry/semantic-conventions#205).Given that this is new 8.0 feature, we should adapt to the spec in 8.0 to avoid breaking changes with 9.0.