Inconsistent status message vs description
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.
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.
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.
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
From what I understand the https://github.com/open-telemetry/opentelemetry-proto/pull/362 using a graceful approach didn't make everyone happy, so we don't want to follow that approach anymore.
We can rename
messagetodescriptionin 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.
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.
Can we just change the name of the term in the spec from
DescriptiontoMessage? 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.
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.
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
descriptionin the API is the same asmessagein 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.
Another approach is to say in the specs Message == Description, a.k.a define an "alias" for the spec concept :)
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 https://github.com/open-telemetry/opentelemetry-proto/issues/431
We need a resolution for this before 1.0. Adding to the milestone.
@tigrannajaryan please assign to me, I'll draft a proposal.