Modify Span status according to specification #224

tigrannajaryan · 2020-09-25T17:58:42Z

Renamed Status.code field to Status.deprecated_code.
Renamed StatusCode enum to Status.DeprecatedStatusCode.
Introduce new Status.code with corresponding StatusCode enum.
Added guidelines about how to use the fields to ensure backward compatibility.

opentelemetry/proto/trace/v1/trace.proto

jkwatson

Looks great!

bryannaegele

How does this avoid being a breaking change of a stable API when the name of the field and its values are being changed?

Oberon00 · 2020-09-27T14:30:36Z

Also, since JSON is allowed, renaming is not even wire-compatible.

bogdandrutu · 2020-09-28T03:02:25Z

JSON is not stable

anuraaga · 2020-09-28T04:14:51Z

opentelemetry/proto/trace/v1/trace.proto

+  //
+  // This field is deprecated and is replaced by `code` field below. See backward
+  // compatibility notes below. According to our stability guarantees this field
+  // will be removed in 12 months, on Sep 26, 2021. All usage of old senders and


Not related to this PR but just noticed since it's defined very clearly here with a date - 12 months seems like a pretty short time for API stability post-GA.

12 months is our formal definition of "stable": https://github.com/open-telemetry/community/blob/47813530864b9fe5a5146f466a58bd2bb94edc72/maturity-matrix.yaml#L57

I think we need a new stability called "GA", that has stronger requirements.

opentelemetry/proto/trace/v1/trace.proto

tigrannajaryan · 2020-09-28T15:49:53Z

How does this avoid being a breaking change of a stable API when the name of the field and its values are being changed?

Names of the fields are not exposed in OpenTelemetry API. Compatibility from protocol perspective means wire compatibility. Names of the fields do not exist on the wire level.

We could keep the name of the field and of the enum values if there is a big desire to keep compatibility on the build level. I am not sure it is worth it and it was not part of the compatibility promise in the past (Collector codebase hides these details on purpose so that usage of OTLP protobuf messages in the codebase is not affected).

I am open to re-considering this and it does not affect the logic.

Oberon00 · 2020-09-28T15:52:16Z

Names of the fields are not exposed in OpenTelemetry API. Compatibility from protocol perspective means wire compatibility. Names of the fields do not exist on the wire level.

What about OTLP/JSON?

tigrannajaryan · 2020-09-28T16:44:48Z

Names of the fields are not exposed in OpenTelemetry API. Compatibility from protocol perspective means wire compatibility. Names of the fields do not exist on the wire level.

What about OTLP/JSON?

Yes, in OTLP/JSON the Protobuf field names are directly exposed on the wire. However, OTLP/JSON is currently in "Alpha", we are free to change it:

bryannaegele · 2020-09-28T19:18:01Z

@tigrannajaryan tooling does rely on these names, so it is a breaking change for every library that relies on the proto names to have to update. When these files (Common and Trace) were officially marked stable in August, I assumed that included stable for tooling, as well.

What I'm not clear on is why this enum is now being deprecated at all, nor why it's being marked as deprecated by prefixing the word to everything. The comment directly above this code states The Status type defines a logical error model that is suitable for different programming environments, including REST APIs and RPC APIs. but now those codes are marked deprecated and not replaced by anything equivalent.

It's unclear to me in https://github.com/open-telemetry/oteps/blob/master/text/trace/0136-error_flagging.md#status-mapping-schema if these codes are actually intended to be removed at this time.

As part of the specification, OpenTelemetry provides a mapping of semantic conventions to status codes. This removes any ambiguity as to what OpenTelemetry ships with out of the box.

What I did get from the OTEP was the addition of a code field Status.Code which has been named Status.StatusCanonicalCode in here and the addition of a Status.Source` field which seems to not be in the PR.

I could be missing something as it's difficult to keep up. If so, I'd love to have more context on this PR. I greatly welcome the additions but I'm unclear on the motivation of the deprecation.

tigrannajaryan · 2020-09-28T20:17:06Z

It's unclear to me in https://github.com/open-telemetry/oteps/blob/master/text/trace/0136-error_flagging.md#status-mapping-schema if these codes are actually intended to be removed at this time.

@bryannaegele the OTEP's language may not be clear but the specification is clear: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#status

There is only one code - the StatusCanonicalCode, there is no other code in the spec that we need to also support in the protocol.

There is certainly no intent to have 2 different codes in the API or in the protocol. The OTEP's motivation was that the old enum did not make sense. This has been discussed in multiple meetings and in the OTEP.

tooling does rely on these names, so it is a breaking change for every library that relies on the proto names to have to update. When these files (Common and Trace) were officially marked stable in August, I assumed that included stable for tooling, as well.

I am not strongly opposed to keeping the old field name and old enum name and value names. The reason I used "deprecated_" as a prefix was to discourage its usage.

If there is no strong oposition I am happy to modify this PR to restore the names of the deprecated field and use new names for the new field and enum.

bryannaegele · 2020-09-28T21:43:46Z

@tigrannajaryan That doc changed 7 days ago where the API spec clearly stated that these status codes were to be used and thus available to other uses, such as messaging https://github.com/open-telemetry/opentelemetry-specification/blame/3e66e6fcc3bb8070db7ccbdd30005ae9f35ac29c/specification/trace/semantic_conventions/http.md#L40-L63. So the ability to set the status with these semantic conventions were in the API spec and does introduce a breaking change.

These status codes being set are also still in the referenced document for HTTP clients https://github.com/open-telemetry/opentelemetry-specification/blame/master/specification/trace/semantic_conventions/http.md#L109-L116.

Pardon my confusion, but this does seem more than a change to the protos and in fact represents a breaking API change, as well. The Commons were marked stable, so I hope you can understand why I'm questioning these changes.

bryannaegele · 2020-09-28T21:47:22Z

If there is no strong oposition I am happy to modify this PR to restore the names of the deprecated field and use new names for the new field and enum.

If this change must be done for some reason, I'm simply asking what is the replacement for marking statuses on a span beyond error? Are users meant to devise their own schemes going forward?

bryannaegele · 2020-09-28T22:22:49Z

I just caught up and found https://github.com/open-telemetry/opentelemetry-specification/pull/966/files from 4 days ago, so this conversation is apparently moot and we'll just have to deal with the breaking changes.

tigrannajaryan · 2020-09-29T00:14:50Z

I'm simply asking what is the replacement for marking statuses on a span beyond error?

The updated spec does not define anything for this. These has been discussed extensively in the spec SIG and error SIG. This PR is a consequence, it is not something new. This simply makes the corresponding change in the protocol.

we'll just have to deal with the breaking changes.

Again, I am happy to keep the old field names in the protocol, but language SDK's have to deal with the spec change anyway. OTLP exporters in SDK will also have to comply with Compatibility requirements for senders that I added in this PR before GA. I don't think we want to release an SDK that has outdated understanding of Span Status.

bryannaegele · 2020-09-29T01:19:32Z

We can't all be in every SIG meeting :)

We'll deal with it. As you noted, we're going to have to remove all this from the SDK anyhow, so it's better to just get the pain over with.

I'm just looking for guidance at this point on how to guide users on how to substitute the functionality. For users that are already leveraging this functionality, what is the upgrade path for them? Are we just saying "Hey, error status codes aren't a thing anymore so you'll have to figure something out" or is there guidance/patterns we can provide? Asking as a user and a maintainer 😆

Oberon00 · 2020-09-29T10:05:01Z

Maybe we should add a compatibility semantic convention to ease transition at least?

tigrannajaryan · 2020-09-29T17:26:09Z

I'm just looking for guidance at this point on how to guide users on how to substitute the functionality. For users that are already leveraging this functionality, what is the upgrade path for them? Are we just saying "Hey, error status codes aren't a thing anymore so you'll have to figure something out" or is there guidance/patterns we can provide? Asking as a user and a maintainer 😆

@bryannaegele best to ask this in spec repo or gitter where it will get more eyeballs.

tigrannajaryan · 2020-10-05T17:12:39Z

Maybe we should add a compatibility semantic convention to ease transition at least?

@Oberon00 Can you please clarify what do you mean by this?

(I was about to merge this PR, just noticed you comment, so will hold for now).

tigrannajaryan · 2020-10-07T15:23:46Z

I have put a bit more thought in to this and I believe the transition in codebases will be easier to do if we keep the names of the deprecated field and enum unchanged. I am going to revert that part of the change. @bryannaegele FYI.

I am going to also wait for open-telemetry/opentelemetry-specification#1069 resolution and do it in one PR. There is no point in introducing the field with wrong names and risk someone start using it and then immediately break it.

tigrannajaryan · 2020-10-09T13:16:51Z

I updated this PR and removed "canonical" adjective to be inline with open-telemetry/opentelemetry-specification#1081

I am using "code value" since "code" is unfortunately already used by a deprecated field which have to keep around for now.

tigrannajaryan · 2020-10-16T19:26:40Z

I would like to know what others think about the field names. The new name looks awkward to me. I would rather prefer to rename the old fields and use StatusCode for the new field/enum.

I am going to open an unrelated PR which legitimizes field renaming as soon as it does not break wire compatibility (which is fine up until we say OTLP/JSON is stable).

tigrannajaryan · 2020-10-16T19:34:44Z

FYI, regarding field name guarantees: #225

tigrannajaryan · 2020-10-20T17:03:47Z

Given that #225 got several approvals and is about to be merged I am inclined more and more to rename the fields since we no longer are limited by the compatibility guarantees. I am going to that unless I hear objections.

1. Renamed Status.code field to Status.deprecated_code. 2. Renamed Status.code.StatusCode enum to Status.DeprecatedStatusCode. 3. Introduce new Status.code with corresponding StatusCanonicalCode enum. 4. Added guidelines about how to use the fields to ensure backward compatibility.

We want to keep the old field and enum names.

This reverts commit b3b01e1.

This reverts commit 6b9a7a4.

opentelemetry/proto/trace/v1/trace.proto

tigrannajaryan · 2020-10-22T16:05:46Z

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers please review. I made a few changes back and forth in this PR. The PR description now reflects the latest state.

tigrannajaryan · 2020-10-26T14:19:45Z

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers please review. Implementation in the Collector cannot proceed until this is merged. The Collector is currently non-compliant with the spec because of this.

Oberon00 · 2020-10-26T14:27:41Z

I think there are already enough approvals, aren't there?

tigrannajaryan · 2020-10-26T15:40:38Z

I think there are already enough approvals, aren't there?

@Oberon00 I re-requested reviews. Approvers previously saw and approved a different version of this PR and I think differences are pretty significant, so it is important for them to confirm they are still OK.

Oberon00 · 2020-10-26T16:20:39Z

@tigrannajaryan Then you should dismiss the reviews in question to make this clearer.

PR is significantly changed, please re-review.

tigrannajaryan · 2020-10-27T12:40:50Z

@arminru @anuraaga @jkwatson PTAL, I need to move forward, this is blocking Collector work. If I don't see any objections I will merge this today.

tigrannajaryan requested review from a team September 25, 2020 17:58

tigrannajaryan force-pushed the feature/tigran/status-code branch from 3544d3a to f2a978e Compare September 25, 2020 18:00

bogdandrutu approved these changes Sep 25, 2020

View reviewed changes

opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved

opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved

jkwatson previously approved these changes Sep 25, 2020

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/status-code branch from f2a978e to 77fffd8 Compare September 25, 2020 18:11

iNikem approved these changes Sep 27, 2020

View reviewed changes

bryannaegele reviewed Sep 27, 2020

View reviewed changes

anuraaga previously approved these changes Sep 28, 2020

View reviewed changes

arminru previously approved these changes Sep 28, 2020

View reviewed changes

MrAlias mentioned this pull request Sep 30, 2020

Update codes to match specification open-telemetry/opentelemetry-go#1214

Merged

tigrannajaryan added the WIP label Oct 7, 2020

tigrannajaryan changed the title ~~Modify Span status according to specification~~ [WIP] [DON'T MERGE] Modify Span status according to specification Oct 7, 2020

Tigran Najaryan added 7 commits October 22, 2020 11:58

Address PR comments

5e88c59

Revert renaming of code to deprecated_code

fd140af

We want to keep the old field and enum names.

Get rid of "canonical" adjective

42cc74c

Revert "Get rid of "canonical" adjective"

f2eb7fa

This reverts commit b3b01e1.

Revert "Revert renaming of code to deprecated_code"

95ee9fd

This reverts commit 6b9a7a4.

Get rid of "canonical" adjective

bc0050f

tigrannajaryan force-pushed the feature/tigran/status-code branch from 53cd0a6 to bc0050f Compare October 22, 2020 15:58

Update removal date

d21654b

Oberon00 reviewed Oct 22, 2020

View reviewed changes

opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved

opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved

Change SHOULD->MUST, move comment section to the top.

3edd5e7

Oberon00 approved these changes Oct 22, 2020

View reviewed changes

drexler mentioned this pull request Oct 23, 2020

Error Flagging with Status Codes open-telemetry/opentelemetry-rust#234

Closed

bogdandrutu approved these changes Oct 24, 2020

View reviewed changes

jkwatson approved these changes Oct 27, 2020

View reviewed changes

tigrannajaryan merged commit 59c488b into open-telemetry:master Oct 27, 2020

tigrannajaryan deleted the feature/tigran/status-code branch October 27, 2020 18:39

ndajr mentioned this pull request Nov 17, 2020

Update otlp exporter to be in sync with changes on opentelemetry-colletor v0.14.0 open-telemetry/opentelemetry-go#1342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Span status according to specification #224

Modify Span status according to specification #224

tigrannajaryan commented Sep 25, 2020 •

edited

Loading

jkwatson left a comment

bryannaegele left a comment

Oberon00 commented Sep 27, 2020

bogdandrutu commented Sep 28, 2020

anuraaga Sep 28, 2020

tigrannajaryan Sep 28, 2020

bogdandrutu Sep 28, 2020

tigrannajaryan commented Sep 28, 2020 •

edited

Loading

Oberon00 commented Sep 28, 2020

tigrannajaryan commented Sep 28, 2020

bryannaegele commented Sep 28, 2020 •

edited

Loading

tigrannajaryan commented Sep 28, 2020

bryannaegele commented Sep 28, 2020 •

edited

Loading

bryannaegele commented Sep 28, 2020

bryannaegele commented Sep 28, 2020

tigrannajaryan commented Sep 29, 2020 •

edited

Loading

bryannaegele commented Sep 29, 2020

Oberon00 commented Sep 29, 2020

tigrannajaryan commented Sep 29, 2020

tigrannajaryan commented Oct 5, 2020

tigrannajaryan commented Oct 7, 2020

tigrannajaryan commented Oct 9, 2020 •

edited

Loading

tigrannajaryan commented Oct 16, 2020

tigrannajaryan commented Oct 16, 2020

tigrannajaryan commented Oct 20, 2020

tigrannajaryan commented Oct 22, 2020

tigrannajaryan commented Oct 26, 2020

Oberon00 commented Oct 26, 2020

tigrannajaryan commented Oct 26, 2020

Oberon00 commented Oct 26, 2020

tigrannajaryan commented Oct 27, 2020

Modify Span status according to specification #224

Modify Span status according to specification #224

Conversation

tigrannajaryan commented Sep 25, 2020 • edited Loading

jkwatson left a comment

Choose a reason for hiding this comment

bryannaegele left a comment

Choose a reason for hiding this comment

Oberon00 commented Sep 27, 2020

bogdandrutu commented Sep 28, 2020

anuraaga Sep 28, 2020

Choose a reason for hiding this comment

tigrannajaryan Sep 28, 2020

Choose a reason for hiding this comment

bogdandrutu Sep 28, 2020

Choose a reason for hiding this comment

tigrannajaryan commented Sep 28, 2020 • edited Loading

Oberon00 commented Sep 28, 2020

tigrannajaryan commented Sep 28, 2020

bryannaegele commented Sep 28, 2020 • edited Loading

tigrannajaryan commented Sep 28, 2020

bryannaegele commented Sep 28, 2020 • edited Loading

bryannaegele commented Sep 28, 2020

bryannaegele commented Sep 28, 2020

tigrannajaryan commented Sep 29, 2020 • edited Loading

bryannaegele commented Sep 29, 2020

Oberon00 commented Sep 29, 2020

tigrannajaryan commented Sep 29, 2020

tigrannajaryan commented Oct 5, 2020

tigrannajaryan commented Oct 7, 2020

tigrannajaryan commented Oct 9, 2020 • edited Loading

tigrannajaryan commented Oct 16, 2020

tigrannajaryan commented Oct 16, 2020

tigrannajaryan commented Oct 20, 2020

tigrannajaryan commented Oct 22, 2020

tigrannajaryan commented Oct 26, 2020

Oberon00 commented Oct 26, 2020

tigrannajaryan commented Oct 26, 2020

Oberon00 commented Oct 26, 2020

tigrannajaryan commented Oct 27, 2020

tigrannajaryan commented Sep 25, 2020 •

edited

Loading

tigrannajaryan commented Sep 28, 2020 •

edited

Loading

bryannaegele commented Sep 28, 2020 •

edited

Loading

bryannaegele commented Sep 28, 2020 •

edited

Loading

tigrannajaryan commented Sep 29, 2020 •

edited

Loading

tigrannajaryan commented Oct 9, 2020 •

edited

Loading