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

Ensure enums are consistently declared #397

Conversation

tigrannajaryan
Copy link
Member

Resolves #363

The rules are:

  • Place enums outside the messages.
  • Prefix the enum value names with enum name, using underscores with uppercase.

The changes in this PR should be backwards compatible (or allowed because
they are in experimental parts):

  • For Binary Protobuf: the encoding does not use enum names anywhere on the wire.
    There are no changes on the wire at all.
  • For JSON Protobuf:
    • DataPointFlags and LogRecordFlags are not visible on the wire since they are
      just helper enums to be used in the code.
    • Moving SpanKind and StatusCode out of the message should not result in any
      changes on the wire.
    • ConstantDecision is experimental and we are free to break it.

Note: all of these changes are breaking for the code that consumes the generated
proto files. We consider this acceptable since this repository is not yet declared
Stable.

I would like someone to independently confirm my analysis to make sure
this PR does not break anything on the wire (neither for binary nor for JSON).

@tigrannajaryan tigrannajaryan requested review from a team May 27, 2022 17:38
Resolves open-telemetry#363

The rules are:
- Place enums outside the messages.
- Prefix the enum value names with enum name, using underscores with uppercase.

The changes in this PR should be backwards compatible (or allowed because
they are in experimental parts):
- For Binary Protobuf: the encoding does not use enum names anywhere on the wire.
  There are no changes on the wire at all.
- For JSON Protobuf:
  - DataPointFlags and LogRecordFlags are not visible on the wire since they are
    just helper enums to be used in the code.
  - Moving SpanKind and StatusCode out of the message should not result in any
    changes on the wire.
  - ConstantDecision is experimental and we are free to break it.

Note: all of these changes are breaking for the code that consumes the generated
proto files. We consider this acceptable since this repository is not yet declared
Stable.

I would like someone to independently confirm my analysis to make sure
this PR does not break anything on the wire (neither for binary nor for JSON).
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/rename-enums branch from 0b81b16 to be5cd5c Compare May 31, 2022 16:36
@Aneurysm9
Copy link
Member

Do we need to do this? What value does it provide?

We saw when we renamed InstrumentationLibrary to InstrumentationScope that even though this repository has attempted to define "stable" in a way that would allow this change, the reality is that the generated identifiers are used in ways that expect them to be truly stable. We have many languages with stable trace and metrics systems that use these definitions because they were told that they were stable. We should have a very high bar for making disruptive name changes at this point and I don't think that this meets that bar.

@tigrannajaryan
Copy link
Member Author

Do we need to do this? What value does it provide?

Consistency in enum names and how they are defined. I don't know if this is enough to justify the pains, but I think it is now or never since the names are also going to be hard-coded in JSON format.

We saw when we renamed InstrumentationLibrary to InstrumentationScope that even though this repository has attempted to define "stable" in a way that would allow this change, the reality is that the generated identifiers are used in ways that expect them to be truly stable. We have many languages with stable trace and metrics systems that use these definitions because they were told that they were stable. We should have a very high bar for making disruptive name changes at this point and I don't think that this meets that bar.

To ease the transition we can try to keep the old enums/names in deprecated form. Would that help?

Also, after this change we can also decide if we want to make our stability guarantees to apply to the generated code. I think we can do it together with JSON stability (since it forces stability of many generated symbols anyway).

@Aneurysm9
Copy link
Member

To ease the transition we can try to keep the old enums/names in deprecated form. Would that help?

Maybe? Since they would be duplicate values and not change message structure that would be less likely to be disruptive, but I think that it could simply lead to carrying the old names forever which would be the worst of all worlds. I think that whatever we do we need to make the versioning and stability policy for the proto and generated code align with the reality that it is viewed as stable and used as if it will not change in backward incompatible ways.

We have made great fanfare about the stability of protocols and data models and it is now time to back that with a v1.0.0 declaration. Now is not the time to allow the perfect to be the enemy of the good. We can spend ages making tweaks here and there or we can acknowledge that we have done a good job and are ready to move on to the next stage in the lifecycle.

Also, after this change we can also decide if we want to make our stability guarantees to apply to the generated code. I think we can do it together with JSON stability (since it forces stability of many generated symbols anyway).

I'm not sure we shouldn't make that decision now. Do we really need to make this change? What will it truly impact? Are things in an unusable state right now or have we somehow managed to live with some minor inconsistencies and it truly is good enough. I fear that we are at risk of losing trust if we make further breaking changes to a data model and protocol we have declared stable.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-proto that referenced this pull request Jun 21, 2022
The changes in this PR do not affect wire formats in any way
since these are helper enums. This is not a breaking change
for the protocol.

This is a breaking change for the generated code.

This change is the smaller subset of open-telemetry#397
@tigrannajaryan
Copy link
Member Author

I submitted only helper enum changes as a separate PR #405 to see if we can at least agree on that part.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-proto that referenced this pull request Jun 21, 2022
The changes in this PR do not affect wire formats in any way
since these are helper enums. This is not a breaking change
for the protocol.

This is a breaking change for the generated code.

This change is the smaller subset of open-telemetry#397
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-proto that referenced this pull request Jun 21, 2022
The changes in this PR do not affect wire formats in any way
since these are helper enums. This is not a breaking change
for the protocol.

This is a breaking change for the generated code.

This change is the smaller subset of open-telemetry#397
@tigrannajaryan
Copy link
Member Author

Closing since we can't find consensus and there are no strong arguments and favour of the PR.

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

Successfully merging this pull request may close these issues.

Revisit enum value names
4 participants