opentelemetry-proto
opentelemetry-proto copied to clipboard
open-telemetry
Just for maintainers information
The way to handle json breaking changes implemented in https://github.com/open-telemetry/opentelemetry-proto/pull/362 caused lots of issues (mainly because existing code continue to compile and in some cases users code upgrade the proto independently of the SDK see go case, or in other cases maintainers did not do the transition because code was not marked as deprecated see swift case), the recommended way is "json_name" annotation (see https://developers.google.com/protocol-buffers/docs/proto3)
After the discussion with @bryce-b the summary is:
- In v0.15.0 we tried to be smarter and instead of using "json_name" annotation (because we did not know about), we kept the same name of the field with a "fake" id (a proto ID that later we reserved) and mark that name "instrumentationLibrarySpans" as deprecated. In the comment we asked users to not send that value when upgrading to the new version.
- The SWIFT protobuf generator does not implement the "deprecated" annotation (or the compiler that they use in the swift repo). Because of this the maintainers of the swift repo did not get any notification to not continue to use that field, and they upgraded and started to send data with the new "fake" id. Here we also broke SWIFT, since if users send data to old collectors the "fake" id would be dropped.
- In the v0.19.0 proto we removed the "smartness" that we added, and hence the "fake" id is no longer accepted in the collector, so we broke SWIFT again.
If Swift maintainers have payed attention to this repo and the deprecation notice they would have avoided both their problems since they also broke when upgraded to v0.15.0 because old collectors were not able to read the new "fake" ID.
I don't think that this is Swift maintainers mistake, but a bad coincidence that the generator does not generate deprecated fields, and that they could not get signaled that they were using a "fake" deprecated id.
Also the issue with "Python" and "Go" were both related to us trying to be smarter and keeping the old name with the fake ID, since instead of causing a compilation error, users were able to build applications with older API/SDK and newer Protos, so they started to send by mistake the new "fake" id (code compiled, but not as expected) that we added and collector was not ready to receive that new "fake" id and translate it into the old id.
This would have been avoidable (and SHOULD) if the maintainers of these languages would not expose the proto generated files as a separate artifact and allow their users to upgrade these independently.
nothing we can do now.
@bogdandrutu Perhaps reintroduction of the backcompat "smartness" in opentelemetry collector could be considered? (at least until proto clients 0.18 can be considered "old enough to not support"). We did exactly that in our fork: https://github.com/fluxninja/opentelemetry-proto/commit/1b57bade656bae78cb672b392f140d0686606b02, https://github.com/fluxninja/opentelemetry-collector/commit/b82d435020ec5e510ab2e5f275e808a0a0940176.
@krdln please file a bug there.
Added also a hack, see https://github.com/open-telemetry/opentelemetry-collector/pull/6342
This would have been avoidable (and SHOULD) if the maintainers of these languages would not expose the proto generated files as a separate artifact and allow their users to upgrade these independently.
To prevent this a separate issue is filed to discuss this policy: https://github.com/open-telemetry/opentelemetry-specification/issues/3420
