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

Don't set Span.Status for 4xx http status codes for SERVER spans #1998

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Oct 6, 2021

Fixes #1818

In addition to all the reasons given in #1818 we have numerous complaints from end-users who are frustrated by seeing all those "error" traces that they can do nothing about.

Changes

Don't set Span.Status for 4xx http status codes for SERVER spans

@iNikem iNikem requested review from a team October 6, 2021 09:26
@Oberon00
Copy link
Member

Oberon00 commented Oct 6, 2021

From a theoretical perspective, I'm weakly against this change, but since I don't have user feedback, I yield. 😀

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

I am weakly in favour of the proposed change (4xx not a server error), however I understand that for some people the opposite is desirable. Unfortunately I cannot think of definitive arguments in favour or against either approach. If we cannot reach consensus on this we can make this behavior configurable.

@Oberon00 Oberon00 added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Oct 6, 2021
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@Oberon00
Copy link
Member

Oberon00 commented Oct 6, 2021

If we cannot reach consensus on this we can make this behavior configurable.

That is the one thing I definitely don't want. That configurability should be on the backend.

@carlosalberto
Copy link
Contributor

@iNikem CHANGELOG entry please, as hopefully maintainers can see this upcoming change (and update all their instrumentation) ;)

@tedsuo
Copy link
Contributor

tedsuo commented Oct 11, 2021

@Oberon00 I disagree about configuration. We need to define a configuration language for OpenTelemetry instrumentation, to handle these issues. But this should be done thoughtfully, as part of the spec. Configuration for instrumentation should not be implemented in an ad-hoc manner; all instrumentation we provide should use the same configuration language.

The instrumentation SIG is working on this configuration issue, but we would greatly appreciate more participation from core Spec members.

As far as this PR goes, I'm fine with the change. My request is that there is some follow up with contrib maintainers and the instrumentation SIG to make sure this change is actually implemented.

@Oberon00
Copy link
Member

Oberon00 commented Oct 11, 2021

@Oberon00 I disagree about configuration. We need to define a configuration language for OpenTelemetry instrumentation, to handle these issues.

I'm not against configurability in general, just in this specific case. For HTTP, as discussed here, the span status is just a function of the http.status_code attribute + span kind. That function can be trivially evaluated with a different configuration on the backend to override the status if desired.

@iNikem
Copy link
Contributor Author

iNikem commented Oct 12, 2021

I propose to have a separate discussion about configurability and its location. This PR got enough approvals and enough time has passed for it to be merged. @open-telemetry/technical-committee do you agree?

@carlosalberto
Copy link
Contributor

@iNikem Definitely let's discuss further details on a follow up. Merging, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Span.Status and http semantic convention
6 participants