Helper enums ZERO value is incorrectly used #433

bogdandrutu · 2022-10-18T01:23:49Z

In the collector before we removed access to the helper enums like DataPointFlags (https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto#L321), I've seen code comparing with FLAG_NONE or initializing as FLAG_NONE the value of the flags to check if a value is present.

If the helper enums contain "masks" then ZERO value does not make sense, if the enums contain "absolute" values, then zero value makes sense but the other values don't make sense.

My personal suggestion is still to remove them (see #423) but if that is not ok, this inconsistency between masks/absolute value has to be fixed.

Assigning to @tigrannajaryan since he is a big supported of these values.

The text was updated successfully, but these errors were encountered:

tigrannajaryan · 2022-10-18T12:43:00Z

If the helper enums contain "masks" then ZERO value does not make sense

@bogdandrutu I think you are right. I support removing the zero value from enums for bitmasks.

tigrannajaryan · 2022-10-18T15:21:56Z

I think this should be done before 1.0 so adding to the milestone.

Aneurysm9 · 2022-10-18T21:43:02Z

I'm not sure what the problem is with the way things are currently. If a helper enum contains bitmask values then a NONE value still makes sense.

const (
	FLAG_NONE = 0
	FLAG_RED = 1
	FLAG_BLUE = 2
	FLAG_GREEN = 4
)

// 7
value := FLAG_NONE | FLAG_RED | FLAG_BLUE | FLAG_GREEN

// false
isNone := value == FLAG_NONE // or, value == value & FLAG_NONE

// true
isRed := FLAG_RED == value & FLAG_RED

//true
isGreen := FLAG_GREEN == value & FLAG_GREEN

//true
isBlue := FLAG_BLUE == value & FLAG_BLUE

While it can allow for nonsensical combinations, this is likely the case for many bitmask-type flag sets and there are still sensible ways to interact with them. Is a user required to utilize the values correctly? Yes. That's the case regardless whether we include helper enums or not, though.

Are there concrete examples of these values being used incorrectly that can be pointed out? I think in their absence it will be difficult to progress here.

bogdandrutu · 2022-10-18T23:47:31Z

What is the meaning of "None" in a bit map? Why do you not offer a "All" (or other subset)?

Let's assume you have a 3-state flag somewhere with properties "foo/bar/baz" and the mask will be "6". what None means?

tigrannajaryan · 2022-10-19T14:20:16Z

I think the check bitmask_field == FLAG_NONE is often used as a proxy for (bitmask_field & FLAG_RED == 0) && (bitmask_field & FLAG_GREEN == 0) && (bitmask_field & FLAG_BLUE = 0).

It is an incorrect usage because bitmask_field typically contains reserved bits which can be allocated in the future and these two checks will no longer have the same meaning.

tigrannajaryan · 2023-04-28T17:00:40Z

I looked into this and unfortunately am unable to fix it.

Protobufs require that enums start with a 0 value.

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.

If anyone knows how to fix this please let me know, otherwise I am going to close this issue.

tigrannajaryan · 2023-04-28T17:01:05Z

@open-telemetry/specs-approvers If anyone knows how to fix this please let me know, otherwise I am going to close this issue (see comment above).

jmacd · 2023-05-02T15:18:37Z

I feel we are going too far. Let's just let there be a FLAG_NONE and carefully document that it is a bitmask value.

jmacd · 2023-05-02T15:32:59Z

For the record, I think we should keep every existing enum as-is and declare 1.0.

Resolves open-telemetry#433 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 } ``` 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.

Resolves open-telemetry#433 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 } ``` 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. 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. ```

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 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](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. ```

tigrannajaryan · 2023-05-02T16:01:41Z

I created a PR that discourages the use of zero values for bitfields: #461

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 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](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. ```

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. ```

yurishkuro · 2023-05-10T17:38:12Z

As others have pointed out, I am not really seeing why mask == NONE is incorrect as a general statement.

In some cases the mask represents a grouping of unrelated things (e.g. the Flags field in Traceparent has bits for is_sampled and is_id_random). In those scenarios asking a question mask == 0 has no semantic meaning imo.
But in other cases bitmasks are used as a quick check for set members, e.g. you have a group of callbacks / handlers for a given instrumentation point and you want to control via configuration which handlers are allowed in a given locus. In this scenario the question mask == 0 makes total sense as you can short circuit the whole loop, and it is completely reasonable to write the question as mask == NONE.

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. ```

tigrannajaryan · 2023-05-10T18:55:11Z

As others have pointed out, I am not really seeing why mask == NONE is incorrect as a general statement.

In some cases the mask represents a grouping of unrelated things (e.g. the Flags field in Traceparent has bits for is_sampled and is_id_random). In those scenarios asking a question mask == 0 has no semantic meaning imo.

But in other cases bitmasks are used as a quick check for set members, e.g. you have a group of callbacks / handlers for a given instrumentation point and you want to control via configuration which handlers are allowed in a given locus. In this scenario the question mask == 0 makes total sense as you can short circuit the whole loop, and it is completely reasonable to write the question as mask == NONE.

I think the reasoning is that we have seen enough cases of a mistake described in the first bullet that we think it is OK to sacrifice the convenience you get for the second bullet's case.

Resolves #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. ```

bogdandrutu added the bug Something isn't working label Oct 18, 2022

bogdandrutu assigned tigrannajaryan Oct 18, 2022

tigrannajaryan added this to the Declare OTLP 1.0 milestone Oct 18, 2022

tigrannajaryan mentioned this issue Apr 20, 2023

Mark opentelemetry-proto repo 1.0 #456

Closed

7 tasks

tigrannajaryan mentioned this issue May 2, 2023

Clarify that zero values bitfield helper enums should not be used #461

Merged

tigrannajaryan closed this as completed in #461 May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helper enums ZERO value is incorrectly used #433

Helper enums ZERO value is incorrectly used #433

bogdandrutu commented Oct 18, 2022 •

edited

tigrannajaryan commented Oct 18, 2022

tigrannajaryan commented Oct 18, 2022

Aneurysm9 commented Oct 18, 2022

bogdandrutu commented Oct 18, 2022 •

edited

tigrannajaryan commented Oct 19, 2022

tigrannajaryan commented Apr 28, 2023

tigrannajaryan commented Apr 28, 2023

jmacd commented May 2, 2023

jmacd commented May 2, 2023

tigrannajaryan commented May 2, 2023

yurishkuro commented May 10, 2023

tigrannajaryan commented May 10, 2023

Helper enums ZERO value is incorrectly used #433

Helper enums ZERO value is incorrectly used #433

Comments

bogdandrutu commented Oct 18, 2022 • edited

tigrannajaryan commented Oct 18, 2022

tigrannajaryan commented Oct 18, 2022

Aneurysm9 commented Oct 18, 2022

bogdandrutu commented Oct 18, 2022 • edited

tigrannajaryan commented Oct 19, 2022

tigrannajaryan commented Apr 28, 2023

tigrannajaryan commented Apr 28, 2023

jmacd commented May 2, 2023

jmacd commented May 2, 2023

tigrannajaryan commented May 2, 2023

yurishkuro commented May 10, 2023

tigrannajaryan commented May 10, 2023

bogdandrutu commented Oct 18, 2022 •

edited

bogdandrutu commented Oct 18, 2022 •

edited