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

Inconsistent Span status field name: message in OTLP vs description in API #430

Closed
Tracked by #1957
bogdandrutu opened this issue Oct 6, 2022 · 12 comments · Fixed by open-telemetry/opentelemetry-specification#2892
Assignees

Comments

@bogdandrutu
Copy link
Member

In the proto:

https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto#L264

In the specs:

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

Proposal:
We should fix this, but we should not repeat the previous mistake of adding a new field with that name and simply rename. The JSON parser in collector will do the right thing.

@tigrannajaryan
Copy link
Member

We should fix this, but we should not repeat the previous mistake of adding a new field with that name and simply rename. The JSON parser in collector will do the right thing.

What's the proposed fix? One option is to do nothing, keep the field name in proto as is and keep the parameter name in the API as is.

@bogdandrutu
Copy link
Member Author

I would rename, and ask JSON to send both values, and receivers to parse both for 6m, without adding a new field with the old name as we did in case of library -> scope.

@tigrannajaryan tigrannajaryan changed the title Inconsistent status message vs description Inconsistent Span status field name: message in OTLP vs description in API Oct 17, 2022
@tigrannajaryan
Copy link
Member

From what I understand the previous approach of renaming a field using a graceful approach didn't make everyone happy, so we don't want to follow that approach anymore.

What do we do instead?

We can rename message to description in the proto directly without introducing an intermediate field. It is not a breaking change for binary Protobuf. It is a breaking change for OTLP/JSON which we can require OTLP/JSON receivers to handle gracefully for a few months by accepting both the old and new name. It is a breaking change for generated code, which we can consider acceptable (?) and will not try to handle in any graceful manner.

Thoughts?

cc @Aneurysm9 @dyladan

@dyladan
Copy link
Member

dyladan commented Oct 17, 2022

From what I understand the #362 using a graceful approach didn't make everyone happy, so we don't want to follow that approach anymore.

We can rename message to description in the proto directly without introducing an intermediate field. It is not a breaking change for binary Protobuf.

The issue with the previous approach was that it broke too many things. This proposal seems even more breaking so I'm not sure its any better.

It is a breaking change for OTLP/JSON which we can require OTLP/JSON receivers to handle gracefully for a few months by accepting both the old and new name. It is a breaking change for generated code, which we can consider acceptable (?) and will not try to handle in any graceful manner.

The constant breaking of OTLP JSON has to stop or nobody will have confidence in the project. Regardless of what is decided here, I think we need to declare JSON stable as soon as possible. It keeps getting pushed back but at some point we have to say what we have is good enough and fields won't be renamed.


I'm still not clear what happened to swift. Was hoping to get a summary from @bogdandrutu or @Aneurysm9 when it was discovered how the field rename broke swift. It was my understanding that renaming the field but keeping the same id should not have been breaking on the wire. Today in the maintainers call it seems like that might actually have been breaking? It's important because I think we should hold all renames until we know for certain what happened and that it can be prevented.

@Aneurysm9
Copy link
Member

Can we just change the name of the term in the spec from Description to Message? The spec is a human-targeted document and can gracefully deal with comments saying "we used to call it X, now it is Y" much better than protobuf. For most languages I expect changing the parameter name to be a no-op. For languages that do have named parameters it should be possible to allow either and emit a warning or error if both are provided and differ.

@tigrannajaryan
Copy link
Member

Can we just change the name of the term in the spec from Description to Message? The spec is a human-targeted document and can gracefully deal with comments saying "we used to call it X, now it is Y" much better than protobuf. For most languages I expect changing the parameter name to be a no-op. For languages that do have named parameters it should be possible to allow either and emit a warning or error if both are provided and differ.

I think this is worth entertaining. Perhaps we can look into all languages and see if there are any that actually use named parameters.

@tigrannajaryan
Copy link
Member

The constant breaking of OTLP JSON has to stop or nobody will have confidence in the project. Regardless of what is decided here, I think we need to declare JSON stable as soon as possible. It keeps getting pushed back but at some point we have to say what we have is good enough and fields won't be renamed.

This is another option, i.e. do nothing, live with the name discrepancy.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Oct 18, 2022

To summarize, 4 options to resolve this:

  • Do nothing. The names in the proto and in the API remain as they are (inconsistent)
  • Do almost nothing. In the spec say that the description in the API is the same as message in the proto.
  • Change the field name in the proto (whatever exactly that means, breaking or trying to do it gracefully).
  • Change the parameter name in the API (possibly a breaking change in some languages).

I added this to Spec SIG meeting agenda tomorrow.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 18, 2022

Another approach is to say in the specs Message == Description, a.k.a define an "alias" for the spec concept :)

@bogdandrutu
Copy link
Member Author

I'm still not clear what happened to swift. Was hoping to get a summary from @bogdandrutu or @Aneurysm9 when it was discovered how the field rename broke swift. It was my understanding that renaming the field but keeping the same id should not have been breaking on the wire. Today in the maintainers call it seems like that might actually have been breaking? It's important because I think we should hold all renames until we know for certain what happened and that it can be prevented.

See #431

@tigrannajaryan
Copy link
Member

We need a resolution for this before 1.0. Adding to the milestone.

@tigrannajaryan tigrannajaryan added this to the Declare OTLP 1.0 milestone Oct 18, 2022
@Aneurysm9
Copy link
Member

@tigrannajaryan please assign to me, I'll draft a proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants