opentelemetry-proto icon indicating copy to clipboard operation
opentelemetry-proto copied to clipboard

Inconsistent status message vs description

Open bogdandrutu opened this issue 3 years ago • 2 comments

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.

bogdandrutu avatar Oct 06 '22 21:10 bogdandrutu

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.

tigrannajaryan avatar Oct 07 '22 13:10 tigrannajaryan

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.

bogdandrutu avatar Oct 07 '22 16:10 bogdandrutu

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

tigrannajaryan avatar Oct 17 '22 21:10 tigrannajaryan

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 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.

dyladan avatar Oct 17 '22 21:10 dyladan

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.

Aneurysm9 avatar Oct 17 '22 21:10 Aneurysm9

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 avatar Oct 17 '22 21:10 tigrannajaryan

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 avatar Oct 17 '22 22:10 tigrannajaryan

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.

tigrannajaryan avatar Oct 18 '22 00:10 tigrannajaryan

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

bogdandrutu avatar Oct 18 '22 00:10 bogdandrutu

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

bogdandrutu avatar Oct 18 '22 00:10 bogdandrutu

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

tigrannajaryan avatar Oct 18 '22 15:10 tigrannajaryan

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

Aneurysm9 avatar Oct 18 '22 15:10 Aneurysm9