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!: total order constraint on span.status= #805

Merged
merged 9 commits into from
Jun 14, 2021
Merged

Conversation

fbogsany
Copy link
Contributor

@fbogsany fbogsany commented Jun 8, 2021

See open-telemetry/opentelemetry-specification#1685

BREAKING CHANGES

  • The OpenTelemetry::Trace::Util::HttpToStatus module has been removed. It previously contributed the http_to_status class method to the Status class. That method incorrectly set the span status to OK for response codes in the range 100..399. Calls to that method should be changed as follows:
# change:
span.status = OpenTelemetry::Trace::Status.http_to_status(response_code)
# to:
span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response_code.to_i)
  • The Status.new(code, description:) initializer has been hidden in favour of simpler constructors for each status code: Status.ok, Status.error and Status.unset. Each constructor takes an optional description.

robertlaurin
robertlaurin previously approved these changes Jun 8, 2021
@robertlaurin
Copy link
Contributor

Excon is doing something fishy with span status here, probably needs to be updated.

span.status = OpenTelemetry::Trace::Status.http_to_status(
response[:status]
)

@robertlaurin
Copy link
Contributor

If instrumentation is not supposed to set an OK status anymore we probably need to look for uses of this too

module HttpToStatus
# Maps numeric HTTP status codes to Trace::Status. This module is a mixin for Trace::Status
# and is not intended for standalone use.
#
# @param code Numeric HTTP status
# @return Status
def http_to_status(code)
case code.to_i
when 100..399
new(const_get(:OK))
else
new(const_get(:ERROR))
end
end

@robertlaurin robertlaurin self-requested a review June 8, 2021 20:13
@fbogsany
Copy link
Contributor Author

fbogsany commented Jun 8, 2021

I think we need to 👀 the semantic conventions for HTTP instrumentation - I think there was a requirement to set the span status based on the response code.

@robertlaurin
Copy link
Contributor

Span Status MUST be left unset if HTTP status code was in the 1xx, 2xx or 3xx ranges, unless there was another error (e.g., network error receiving the response body; or 3xx codes with max redirects exceeded), in which case status MUST be set to Error.

For HTTP status codes in the 4xx and 5xx ranges, as well as any other code the client failed to interpret, status MUST be set to Error.

Don't set the span status description if the reason can be inferred from http.status_code.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#status

@fbogsany fbogsany linked an issue Jun 10, 2021 that may be closed by this pull request
Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

LGTM!

@fbogsany fbogsany changed the title fix: total order constraint on span.status= fix!: total order constraint on span.status= Jun 14, 2021
@fbogsany fbogsany merged commit 55b6be1 into main Jun 14, 2021
@fbogsany fbogsany deleted the status-total-order branch June 14, 2021 15:32
aquarhead added a commit to PLAIO/opentelemetry-ruby-contrib that referenced this pull request Jan 8, 2025
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.

Consider removing HttpToStatus from API/core
3 participants