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

Update span status & description setting to comply with the spec #1376

Closed
matej-g opened this issue Nov 29, 2020 · 5 comments
Closed

Update span status & description setting to comply with the spec #1376

matej-g opened this issue Nov 29, 2020 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@matej-g
Copy link
Contributor

matej-g commented Nov 29, 2020

Related PRs (merged ones ticked off)


According to recent spec change in open-telemetry/opentelemetry-specification#1186 and
open-telemetry/opentelemetry-specification#1257:

Regarding exporters

For Jaeger status code and message (excerpts):

Span Status MUST be reported as a key-value pair in tags to Jaeger, unless it is UNSET.
In the latter case it MUST NOT be reported.

|Code | otel.status_code | Name of the code, either OK or ERROR. MUST NOT be set if the code is UNSET. |
|Description | otel.status_description | Description of the Status if it has a value otherwise not set. |

For Zipkin status code and description (excerpts):

|Description| error | Description of the Status. MUST be set if the code is ERROR, use an empty string if Description has no value. MUST NOT be set for OK and UNSET codes. |

Regarding status from HTTP status code (open question)

Furthermore, the spec now states that:

Description MUST be IGNORED for StatusCode Ok & Unset values.

and in HTTP semantic conventions spec:

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

Generally this seems to be the case, however SpanStatusFromHTTPStatusCode, which - although not returning directly span status with description - is returning accompanying message for all statuses. The question is whether this acceptable (and the burden of checking and (not) setting span status is on the caller of SpanStatusFromHTTPStatusCode) or this could be simplified to return empty string for success statuses.

@MrAlias MrAlias added the bug Something isn't working label Dec 3, 2020
@MrAlias MrAlias added this to the RC1 milestone Dec 3, 2020
@matej-g
Copy link
Contributor Author

matej-g commented Dec 11, 2020

@MrAlias I updated the description as there was another related change to the spec (see the update description), I think it makes sense to tackle it all in one go. I hope the update is OK, not sure whether it changes priority etc.

Also happy to pick up this issue ⬆️

@matej-g matej-g changed the title Zipkin Exporter: Do not report status if span status is unset Zipkin & Jaeger Exporters: Update span status & description setting to comply with the spec Dec 11, 2020
@matej-g matej-g changed the title Zipkin & Jaeger Exporters: Update span status & description setting to comply with the spec Update span status & description setting to comply with the spec Dec 11, 2020
@matej-g
Copy link
Contributor Author

matej-g commented Feb 26, 2021

Hello @MrAlias! I've been looking into this and I think there are even more (although mostly minor) discrepancies between the exporters and the spec, since I opened this. I think I will split the changes into separate PRs, one for each exporter, for better review process, just FYI.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 26, 2021

@matej-g sounds good 👍

@MrAlias
Copy link
Contributor

MrAlias commented Mar 31, 2021

Adding the following tasks to this to be explicit for what we need in the Jaeger exporter:

  • The status code tag needs to be renamed from "status.code" to "otel.status_code"
  • The status description tag needs to be renamed from "status.message" to "otel.status_description"
  • The status description tag needs to not be set if the status message is not set.

It might be worth splitting this issue into smaller ones.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 31, 2021

Addressing that last comment in #1761

@MrAlias MrAlias closed this as completed Apr 27, 2021
@pellared pellared moved this to Closed in Go: Triage Nov 2, 2023
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
Archived in project
Development

No branches or pull requests

2 participants