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

Fix handling of non-standard HTTP status codes #3966

Merged
merged 3 commits into from Jun 8, 2023

Conversation

pellared
Copy link
Member

@pellared pellared commented Jun 7, 2023

Fixes #3902

Also from https://www.rfc-editor.org/rfc/rfc9110.html#section-15-4

HTTP status codes are extensible. A client is not required to understand the meaning of all registered status codes, though such understanding is obviously desirable. However, a client MUST understand the class of any status code, as indicated by the first digit, and treat an unrecognized status code as being equivalent to the x00 status code of that class.

@pellared pellared changed the title Fix the span statuses set by HTTP instrumentation libraries for unpopular H Fix span statuses set by HTTP instrumentation libraries for unpopular HTTP status codes Jun 7, 2023
@pellared pellared changed the title Fix span statuses set by HTTP instrumentation libraries for unpopular HTTP status codes Fix handling of non-standard HTTP status codes Jun 7, 2023
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #3966 (fd266ef) into main (916ae40) will decrease coverage by 0.3%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3966     +/-   ##
=======================================
- Coverage   79.4%   79.1%   -0.3%     
=======================================
  Files        165     165             
  Lines      10426   10293    -133     
=======================================
- Hits        8284    8151    -133     
  Misses      2006    2006             
  Partials     136     136             
Impacted Files Coverage Δ
...stful/otelrestful/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...gonic/gin/otelgin/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...rilla/mux/otelmux/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...ack/echo/otelecho/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...on.v1/otelmacaron/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...ace/otelhttptrace/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)
...net/http/otelhttp/internal/semconvutil/httpconv.go 100.0% <100.0%> (ø)

@pellared pellared marked this pull request as ready for review June 7, 2023 07:04
@pellared
Copy link
Member Author

pellared commented Jun 7, 2023

FYI @scorpionknifes

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Also from https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-25.html#rfc.section.6.p.2:

The current STD97 is embodied in RFC9110. Don't think it changes the analysis here, but always good to reference current standards over old drafts.

@pellared pellared merged commit 5cc3715 into open-telemetry:main Jun 8, 2023
22 checks passed
@pellared pellared deleted the fix-http-status-handling branch June 8, 2023 06:55
@codyaray
Copy link

Nice, just what we were looking for! Can you cut a new minor release with this? Do you have an ETA?

@MrAlias MrAlias added this to the v0.43.0 milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non standard 4xx sets span status to codes.Error
5 participants