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

Just for maintainers information #431

Open
bogdandrutu opened this issue Oct 17, 2022 · 6 comments
Open

Just for maintainers information #431

bogdandrutu opened this issue Oct 17, 2022 · 6 comments

Comments

@bogdandrutu
Copy link
Member

The way to handle json breaking changes implemented in #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)

@bogdandrutu
Copy link
Member Author

/cc @tigrannajaryan for visibility only :) nothing we can do now.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 18, 2022

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.

@bogdandrutu
Copy link
Member Author

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.

@krdln
Copy link

krdln commented Oct 18, 2022

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: fluxninja@1b57bad, fluxninja/opentelemetry-collector@b82d435.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 18, 2022

@krdln please file a bug there.

Added also a hack, see open-telemetry/opentelemetry-collector#6342

@tigrannajaryan
Copy link
Member

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: open-telemetry/opentelemetry-specification#3420

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

No branches or pull requests

3 participants