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

Removal of InstrumentationLibrary-related fields and messages violates "stable" maturity level #427

Closed
rmungena opened this issue Sep 12, 2022 · 3 comments

Comments

@rmungena
Copy link

Pull request #407 removed InstrumentationLibrary-related fields and messages from proto definitions. While the corresponding message ID is reserved, it is not possible anymore for implementors (e.g. of Collectors), to access messages or fields, that are related to InstrumentationLibrary. This makes it hard to ensure backward-compatibility with old OTLP clients, although all affected protobufs already have a maturity level of stable. The maturity level is defined as follows:

  # "stable" maturity APIs should not introduce backwards-incompatible changes
  # more than once every twelve months, and will make every effort to provide
  # compatibility bridges if at all possible.

(Source)

Using latest protos of this project, makes it impossible for implementors to generate code, that provides access to the removed messages / fields. In Java it is not possible to have multiple versions of the same dependency on the classpath. So, ensuring backward compatibility would even require some custom code to parse old messages. But maybe I'm overseeing something. What is the recommended approach to ensure backward-compatiblity with old OTLP clients after this change?

Wouldn't it be better to keep the removed messages in the protos and mark the referencing fields as deprecated? I understand, that the goal is to minimize legacy stuff in the protos, but all the protos are already marked as "stable".

@tigrannajaryan
Copy link
Member

I assume this is about the field number 1000 in ResourceLogs/ResourceSpans/ResourceMetrics messages.

This field was introduced in a deprecated form with a comment explaining that binary protobuf senders should never set it. The purpose of the field was to make transition for JSON senders easier and the intent was very clear that the field is temporarily added for 3 months, specifically for JSON senders to use (OTLP/JSON is unstable).

Field number 1000 was never part of the stability guarantees and was removed once the notice period was over. Any OTLP binary Protobuf senders that write this field are broken, they are not compliant with OTLP specification, they never were and we are not aiming to ensure backwards compatibility for broken implementations. If there is an old OTLP client that still uses the field it must be updated to use the field number 2 instead (scope_logs/scope_spans/scope_metrics).

I am open to suggestions on how to handle such transitions better in the future (although it is unlikely to happen again since we are now preparing for OTLP 1.0 release which explicitly prohibits symbolic name changes as well, so a change like this will be illegal after OTLP 1.0).

@rmungena
Copy link
Author

@tigrannajaryan thank you for clarification. Based on your explanation and after reading the (removed) comments, I think we can close this issue.

In general the concept of maturity levels is quite confusing. You mentioned that you are preparing for OTLP 1.0 and that this will prohibit symbolic name changes as well. Do you use semantic versioning?

@tigrannajaryan
Copy link
Member

@rmungena I agree, we can do a better job in explaining the OTLP stability. It is current under discussion #400

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

2 participants