Skip to content

Commit

Permalink
Clarify bitfield helper enums
Browse files Browse the repository at this point in the history
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.
```
  • Loading branch information
tigrannajaryan authored and VinozzZ committed Jun 21, 2024
1 parent 8e28477 commit df02c86
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 5 deletions.
15 changes: 13 additions & 2 deletions opentelemetry/proto/logs/v1/logs.proto
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,21 @@ enum SeverityNumber {
SEVERITY_NUMBER_FATAL4 = 24;
}

// Masks for LogRecord.flags field.
// LogRecordFlags is defined as a protobuf 'uint32' type and is to be used as
// bit-fields. Each non-zero value defined in this enum is a bit-mask.
// To extract the bit-field, for example, use an expression like:
//
// (logRecord.flags & LOG_RECORD_FLAG_TRACE_FLAGS_MASK)
//
enum LogRecordFlags {
LOG_RECORD_FLAG_UNSPECIFIED = 0;
// The zero value for the enum. Should not be used for comparisons.
// Instead use bitwise "and" with the appropriate mask as shown above.
LOG_RECORD_FLAG_DO_NOT_USE = 0;

// Bits 0-7 are used for trace flags.
LOG_RECORD_FLAG_TRACE_FLAGS_MASK = 0x000000FF;

// Bits 8-31 are reserved for future use.
}

// A log record according to OpenTelemetry Log Data Model:
Expand Down
8 changes: 5 additions & 3 deletions opentelemetry/proto/metrics/v1/metrics.proto
Original file line number Diff line number Diff line change
Expand Up @@ -334,15 +334,17 @@ enum AggregationTemporality {
// enum is a bit-mask. To test the presence of a single flag in the flags of
// a data point, for example, use an expression like:
//
// (point.flags & FLAG_NO_RECORDED_VALUE) == FLAG_NO_RECORDED_VALUE
// (point.flags & FLAG_NO_RECORDED_VALUE_MASK) == FLAG_NO_RECORDED_VALUE_MASK
//
enum DataPointFlags {
FLAG_NONE = 0;
// The zero value for the enum. Should not be used for comparisons.
// Instead use bitwise "and" with the appropriate mask as shown above.
FLAG_DO_NOT_USE = 0;

// This DataPoint is valid but has no recorded value. This value
// SHOULD be used to reflect explicitly missing data in a series, as
// for an equivalent to the Prometheus "staleness marker".
FLAG_NO_RECORDED_VALUE = 1;
FLAG_NO_RECORDED_VALUE_MASK = 1;

// Bits 2-31 are reserved for future use.
}
Expand Down

0 comments on commit df02c86

Please sign in to comment.