Skip to content

Commit

Permalink
Clarify that zero values bitfield helper enums should not be used
Browse files Browse the repository at this point in the history
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 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.
```
  • Loading branch information
tigrannajaryan committed May 2, 2023
1 parent 46152b8 commit 636263a
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 3 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_UNSPECIFIED = 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
4 changes: 3 additions & 1 deletion opentelemetry/proto/metrics/v1/metrics.proto
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,9 @@ enum AggregationTemporality {
// (point.flags & FLAG_NO_RECORDED_VALUE) == FLAG_NO_RECORDED_VALUE
//
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_UNSPECIFIED = 0;

// This DataPoint is valid but has no recorded value. This value
// SHOULD be used to reflect explicitly missing data in a series, as
Expand Down

0 comments on commit 636263a

Please sign in to comment.