Clarify that zero values bitfield helper enums should not be used #461
Conversation
78a03d9 to
636263a
Compare
636263a to
93a7308
Compare
This is incorrect because it assumes that no value in the flag means no recorded value in the data point, which is ignoring the flag altogether.
This is also wrong. This predicate will be true when the no recorded value flag is not set. This is the opposite of what is expected. The correct check for no recorded value would be this: if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == FLAG_NO_RECORDED_VALUE { // or != 0
// do something when there is no recorded value
}If you really want a mask value to indicate that no non-reserved flags are set then you would want something like this: const NO_RESERVED_FLAGS_MASK = 0 ^ FLAG_NO_RECORDED_VALUE // also xor any other valid flagsSee this demonstration on the playground: https://go.dev/play/p/Rf0DU9CYvmi |
We haven't published this change yet (that is, not release includes this part yet). I'm guessing the plan is to do a release once 1.0 is reached (very soon hopefully). |
Yeah, I think we should have made it clear that Stability guarantees are for 1.0. That was my intent, I had no intent to lock the current repo state as is, while it is still 0.x. |
We didn't publish a release when we stabilized OTLP/JSON either. The relevant spec is stable and we have changed our public communication regarding what "stable" means in a way that makes this proposed change incompatible with the stability guarantees we say we're making.
If that was the intent then merging the stability guarantees change should have been the last thing done and should have been explicitly coupled to the 1.0 version. We have long said that parts of this spec are "stable" even without a 1.0 version number and I expect to be able to rely on that. |
That was a mistake, but we are doing the same for the Spec - if it's not released, it's not accepted. We have even reverted changes (permanently or temporarily). So let's try to do it right this time and do a prompt release once all the 1.0 items are done. |
Yes, it was a mistake. We wanted the symbolic stability for 1.0. I am going to create a PR that clarifies it. I don't think anybody intended to freeze this repo knowing that we have open issues to resolve before marking it 1.0. |
|
Here is a PR to make it clear that symbolic guarantees go into effect only when 1.0 is released: #467 |
|
Now that #467 is merged it is clear that this PR is allowed. |
4e6cf23 to
5196c42
Compare
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
5196c42 to
945c230
Compare
|
All, I removed the |
|
@open-telemetry/specs-approvers this has enough approvals. Please object, otherwise I will merge. |
Resolves #433
Problem
The zero values defined in helper enums encourage incorrect usage of the bitfields.
The typical incorrect code looks like this:
This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The
ifstatement above will be incorrect if any other bit is set.The correct code looks like this:
Solution
To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use.
Alternates Tried
I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked.
Protobufs require that enums start with a 0 value.
If you try to omit it you get a compilation error:
Alternatively, trying to use a dummy name, e.g.:
Fails Objective-C generator:
Also tried declaring it reserved as the doc says should be possible:
but get an error again: