Skip to content
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

[release/8.0] Replace http.error.reason with OTel standard error.type #91996

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 13, 2023

Backport of #91910 to release/8.0

/cc @antonfirsov

Fixes #91909

Customer Impact

Align attribute name and semantics with the OpenTelemetry spec (which was finalized 2023/9/11 - see open-telemetry/semantic-conventions#205) -- http.error.reason attribute has been standardized under name error.type. We need to:

  • Rename http.error.reason to error.type.
  • Report 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.
    • Note: Currently we report 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.

Testing

A test has been added for the new error.type behavior.

Risk

Low. RC1 users experimenting with HttpClient Metrics may experience an inconvenience with the rename. (It's unlikely there are many at this point.)

@ghost
Copy link

ghost commented Sep 13, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #91910 to release/8.0

/cc @antonfirsov

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@antonfirsov antonfirsov added the Servicing-consider Issue for next servicing release review label Sep 13, 2023
@antonfirsov
Copy link
Member

/cc @karelz @artl93 @noahfalk

@antonfirsov antonfirsov added this to the 8.0.0 milestone Sep 13, 2023
@karelz karelz removed the Servicing-consider Issue for next servicing release review label Sep 13, 2023
@antonfirsov
Copy link
Member

The massive failures in System.Test.Types are unrelated, also hit in #91625. Rerunning to get a clean run.

@antonfirsov
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karelz karelz added the Servicing-consider Issue for next servicing release review label Sep 14, 2023
@karelz
Copy link
Member

karelz commented Sep 14, 2023

I approve - it is a new 8.0 feature and we are aligning with latest OpenTelemetry spec update, preventing breaking change in 9.0.
All tests are passing.

@artl93 it is ready for your approval.

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M2 Approved.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 14, 2023
@carlossanlop carlossanlop merged commit 5de5c46 into release/8.0 Sep 14, 2023
107 of 108 checks passed
@carlossanlop carlossanlop deleted the backport/pr-91910-to-release/8.0 branch September 14, 2023 20:20
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants