Remove Span.Status #134

iNikem · 2020-08-19T10:44:09Z

Addresses open-telemetry/opentelemetry-specification#706

text/0134-remove-span-status.md

tigrannajaryan · 2020-08-19T14:42:19Z

text/0134-remove-span-status.md

+
+## Open questions
+
+Neither OpenTelemetry Specification nor Protocol currently assigns any special meaning to the `error` attribute mentioned above


I am not quite sure what happens if Status is removed but we do not (yet) have the "error"-like attribute. How will the errors be indicated? Do we lose this capability? Or the intent is to wait until "error" indicator (whatever it is) is first added to the specification and only after that we remove Status?

Current proposal is to remove status right now, file a follow up issue and work with Errors WG to resolve it. Either via one of the already existing proposals, like open-telemetry/opentelemetry-specification#822, or a new one.

Got it. I see in this OTEP that you already thought through the transition process, which is great, but I need to mention this for visibility: we must be careful with Collector changes. There are people who use OpenTelemetry Collector in production already. All intermediate states of the Collector need to be fully functional.

@tigrannajaryan It seems open-telemetry/opentelemetry-specification#822 will be effectively merged soon (the error.hint Span attribute at least). Would that be enough for the Collector?

I am reluctant to give an approval until I clearly understand what we replace this by.

If open-telemetry/opentelemetry-specification#822 is the replacement can we wait for it be merged first and update this OTEP to make use of it? It is an open question in this OTEP and is one of the central problems that this OTEPs needs to solve, but remains partially unsolved for now.

I think the otep may change as a result of resolving any open question, but that doesn't mean we should stop this. I think the main point of this otep is the removing of span status and an action item is required to think about if we need an equivalent of error.hint or not separately.

Trying to make progress so we don't get stuck, but I can see your point as well, if you suggest that we really need a way to signal error from the application (which I don't believe we need) then I understand why we should block this until we have that.

@tigrannajaryan can you express your opinion in open-telemetry/opentelemetry-specification#822? If you think that it may resolve your hesitation about this PR

@bogdandrutu I think we do need a definitive way to signal errors on the span. Some products visualize this and OpenTracing has the convention for this, OpenCensus protocol has this built-in. I believe we need an equivalent in OpenTelemetry.

@iNikem I support "error.hint" and commented there and beleive the semantic conventions for it need to be merged asap to unblock this OTEP.

I am not against moving forward with this OTEP, but I believe it will have an important open question and will be unactionable in the Collector if it does not take into account either the existence of "error.hint" or the lack of whatever else needs to replace Span Status. IMO, it will mean we will need another OTEP to correct this OTEP once "error.hint" is merged or rejected.

I am not against moving forward with this OTEP, but I believe it will have an important open question and will be unactionable in the Collector if it does not take into account either the existence of "error.hint" or the lack of whatever else needs to replace Span Status. IMO, it will mean we will need another OTEP to correct this OTEP once "error.hint" is merged or rejected.

Not sure you need an OTEP for that, that is a semantic convention, and collector will use it.

@bogdandrutu sure, that probably works too.

andrewhsu · 2020-08-21T15:57:54Z

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers this PR is blocking open-telemetry/opentelemetry-specification#706 so if this is suitable, please leave a LGTM

iNikem · 2020-08-25T09:50:34Z

@open-telemetry/technical-committee Can this be merged now? Then I can proceed doing actual changes.

Oberon00

I have some questions about details, but all in all I agree to remove Status.

text/0134-remove-span-status.md

Oberon00 · 2020-08-25T10:08:11Z

text/0134-remove-span-status.md

+
+* If incoming OTLP data has `Span.Status` present, that will NOT be removed and will be passed on as-is.
+* If incoming OTLP data has `Span.Status` present and it is not equal to `OK`,
+Collector will translate that to temporary semantic attributes as follows:


Is the previous point not enough? Do we need to duplicate the status as attributes?

Yes, because new OTLP receivers will not read status field from the wire. But we still may want to pass this information if it was available in the Collector.

Do we expect code behind new OTLP receivers that don't read (deprecated_)status anymore to still need status?

Don't know, that is way we leave it there for the time being. Collector maintainers may decide when they want to remove this translation.

Oberon00 · 2020-08-25T10:08:32Z

text/0134-remove-span-status.md

+The exact duration of this transition period will be left for Collector's maintainers to decide.
+
+* If incoming OTLP data has `Span.Status` present, that will NOT be removed and will be passed on as-is.
+* If incoming OTLP data has `Span.Status` present and it is not equal to `OK`,


Should a non-empty message also be translated independently of the status code?

Message is part of the Status, how can it be independent?

I mean, it makes sense to add deprecated_status when "incoming OTLP data has Span.Status present and it is not equal to OK", but it may make sense to independently set deprecated_message when "incoming OTLP data has Span.Status present and message non-empty`"

I think having message without code will be confusing, because current state is exactly the opposite: you may have code without message.

But you may also have a message with an OK code. If you are worried about confusion, then set the status code additionally when the message is non-empty.

text/0134-remove-span-status.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Better cross-reference

carlosalberto · 2020-08-25T13:26:10Z

@open-telemetry/specs-approvers Please review this OTEP. We need one more approval to proceed (and it's possible some of you haven't had time to pay attention to check this OTEP out ;) )

tigrannajaryan · 2020-09-01T14:43:15Z

All, I don't want to be the person who blocks this OTEP. My only worry is that "error.hint" PR is not going to be merged due to disagreements and we will remove the Span status without having a reasonable replacement.

If everyone else in this thread thinks that it is not a problem and we will find a solution to that I am happy to approve and go ahead with this OTEP.

tigrannajaryan

Based on the spec SIG meeting discussion I am approving this with an expectation that we will have a resolution for what will replace this functionality after the Error WG meeting this Thursday.

tedsuo · 2020-09-17T16:00:37Z

@iNikem Now that OTEP 136 is merged, this PR may be closed.

iNikem · 2020-09-17T18:22:04Z

Closed in favor of #136

iNikem added 3 commits August 13, 2020 18:11

Remove status

3b50309

OTEP to remove Span.Status

98a7417

Assign OTEP number

16810cc

iNikem marked this pull request as ready for review August 19, 2020 10:46

iNikem requested a review from a team August 19, 2020 10:46

Format

ee19094

ktoso mentioned this pull request Aug 19, 2020

Consider removal of Span.Status, as OTel also removing it slashmo/gsoc-swift-tracing#134

Open

tigrannajaryan reviewed Aug 19, 2020

View reviewed changes

tigrannajaryan mentioned this pull request Aug 19, 2020

Span Status Improvements (without errors) #133

Closed

Polish

36a2b89

bogdandrutu approved these changes Aug 20, 2020

View reviewed changes

jmacd approved these changes Aug 21, 2020

View reviewed changes

carlosalberto self-requested a review August 21, 2020 16:49

carlosalberto approved these changes Aug 21, 2020

View reviewed changes

iNikem mentioned this pull request Aug 24, 2020

Add error.hint semantic convention open-telemetry/opentelemetry-specification#822

Closed

lzchen mentioned this pull request Aug 24, 2020

Review spec for tracing open-telemetry/opentelemetry-python#928

Closed

Oberon00 approved these changes Aug 25, 2020

View reviewed changes

iNikem and others added 3 commits August 25, 2020 13:22

Apply suggestions from code review

82e1d9f

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Update 0134-remove-span-status.md

dd90523

Better cross-reference

Merge branch 'master' into remove_status

acac296

arminru approved these changes Aug 25, 2020

View reviewed changes

Oberon00 approved these changes Aug 25, 2020

View reviewed changes

Aneurysm9 mentioned this pull request Aug 25, 2020

Prepare Release v0.11.0 open-telemetry/opentelemetry-go-contrib#282

Merged

MrAlias mentioned this pull request Aug 26, 2020

Create a codes conversion function for gRPCs instrumentation open-telemetry/opentelemetry-go-contrib#295

Closed

tigrannajaryan approved these changes Sep 1, 2020

View reviewed changes

iNikem mentioned this pull request Sep 3, 2020

Remove Span.Status per OTEP-134 open-telemetry/opentelemetry-specification#918

Closed

vmarchaud mentioned this pull request Sep 4, 2020

fix: Add status exception on Span.recordException open-telemetry/opentelemetry-js#1490

Closed

tedsuo mentioned this pull request Sep 9, 2020

Error flagging with status codes #136

Merged

iNikem closed this Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Span.Status #134

Remove Span.Status #134

iNikem commented Aug 19, 2020 •

edited

Loading

tigrannajaryan Aug 19, 2020

iNikem Aug 19, 2020

tigrannajaryan Aug 19, 2020

carlosalberto Aug 25, 2020

tigrannajaryan Aug 25, 2020

bogdandrutu Aug 30, 2020

iNikem Aug 31, 2020

tigrannajaryan Aug 31, 2020

bogdandrutu Sep 1, 2020

tigrannajaryan Sep 1, 2020

andrewhsu commented Aug 21, 2020

iNikem commented Aug 25, 2020

Oberon00 left a comment

Oberon00 Aug 25, 2020

iNikem Aug 25, 2020

Oberon00 Aug 25, 2020

iNikem Aug 25, 2020

Oberon00 Aug 25, 2020

iNikem Aug 25, 2020

Oberon00 Aug 25, 2020

iNikem Aug 25, 2020

Oberon00 Aug 25, 2020

carlosalberto commented Aug 25, 2020

tigrannajaryan commented Sep 1, 2020

tigrannajaryan left a comment

tedsuo commented Sep 17, 2020

iNikem commented Sep 17, 2020


		## Open questions

		Neither OpenTelemetry Specification nor Protocol currently assigns any special meaning to the `error` attribute mentioned above

Remove Span.Status #134

Remove Span.Status #134

Conversation

iNikem commented Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhsu commented Aug 21, 2020

iNikem commented Aug 25, 2020

Oberon00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Aug 25, 2020

tigrannajaryan commented Sep 1, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

tedsuo commented Sep 17, 2020

iNikem commented Sep 17, 2020

iNikem commented Aug 19, 2020 •

edited

Loading