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

Non standard 4xx sets span status to codes.Error #3902

Closed
scorpionknifes opened this issue May 25, 2023 · 3 comments · Fixed by #3966
Closed

Non standard 4xx sets span status to codes.Error #3902

scorpionknifes opened this issue May 25, 2023 · 3 comments · Fixed by #3966
Assignees
Labels
bug Something isn't working

Comments

@scorpionknifes
Copy link
Member

Description

A http request for a non standard http status code 4xx error (e.g. 499), sets the span status to codes.Errors.
validateHTTPStatusCode causes this issue in http middlewares (e.g. otelecho).

validateHTTPStatusCode returns codes.Errors instead of codes.Unset although the http status is in the 4xx range.
Since 499 is not part of the validRangesPerCategory
It returns codes.Errors and Invalid HTTP status code

validateHTTPStatusCode - https://github.com/open-telemetry/opentelemetry-go/blob/main/semconv/internal/v2/http.go#L386-L399

Steps To Reproduce

  1. Use any middleware relying on validateHTTPStatusCode e.g. otelecho
  2. Call a endpoint that results in a non standard http status code e.g 499
  3. span status = codes.Error

Expected behavior

Error codes such as 499 is in range of 4xx and error should be codes.Unset instead of codes.Error

Alternatively configure it to allow user defined 4xx error codes such as 499?

@scorpionknifes scorpionknifes added the bug Something isn't working label May 25, 2023
@scarshad
Copy link

scarshad commented May 25, 2023

https://github.com/open-telemetry/opentelemetry-go/blob/46f2ce5ca6adaa264c37cdbba251c9184a06ed7f/semconv/internal/v2/http.go#L400-L402

499 is the error status code, and in the error category.

why should it be in category of codes.Unset?

@scorpionknifes
Copy link
Member Author

scorpionknifes commented May 25, 2023

https://github.com/open-telemetry/opentelemetry-go/blob/46f2ce5ca6adaa264c37cdbba251c9184a06ed7f/semconv/internal/v2/http.go#L400-L402

499 is the error status code, and in the error category.

why should it be in category of codes.Unset?

It returns a couple lines ahead at https://github.com/open-telemetry/opentelemetry-go/blob/46f2ce5ca6adaa264c37cdbba251c9184a06ed7f/semconv/internal/v2/http.go#L397-L399

with 499 resulting in codes.Error, I expect it to be codes.Unset

@pellared
Copy link
Member

pellared commented May 25, 2023

I agree that it looks like a bug. Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md#status. For example this is how OTel .NET handles it https://github.com/open-telemetry/opentelemetry-dotnet/blob/eb12f9b386f0f3231bd9f41b34b65cc927831e1c/src/OpenTelemetry.Api/Internal/SpanHelper.cs (it checks against ranges - not specific values).

I can look at it once #3817 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants