Revisit enum value names

[bogdandrutu]

Background

Currently we do not have a consistent way of naming values in the enums:

  enum StatusCode {
    STATUS_CODE_UNSET               = 0;
    STATUS_CODE_OK                  = 1;
    STATUS_CODE_ERROR               = 2;
  };

  enum SpanKind {
    SPAN_KIND_UNSPECIFIED = 0;
    SPAN_KIND_INTERNAL = 1;
    SPAN_KIND_SERVER = 2;
    SPAN_KIND_CLIENT = 3;
    SPAN_KIND_PRODUCER = 4;
    SPAN_KIND_CONSUMER = 5;
  }

enum AggregationTemporality {
  AGGREGATION_TEMPORALITY_UNSPECIFIED = 0;
  AGGREGATION_TEMPORALITY_DELTA = 1;
  AGGREGATION_TEMPORALITY_CUMULATIVE = 2;
}

enum DataPointFlags {
  FLAG_NONE = 0;
  FLAG_NO_RECORDED_VALUE = 1;
}

// Possible values for LogRecord.SeverityNumber.
enum SeverityNumber {
  // UNSPECIFIED is the default SeverityNumber, it MUST NOT be used.
  SEVERITY_NUMBER_UNSPECIFIED = 0;
  SEVERITY_NUMBER_TRACE  = 1;
  SEVERITY_NUMBER_TRACE2 = 2;
  SEVERITY_NUMBER_TRACE3 = 3;
  SEVERITY_NUMBER_TRACE4 = 4;
  SEVERITY_NUMBER_DEBUG  = 5;
  SEVERITY_NUMBER_DEBUG2 = 6;
  SEVERITY_NUMBER_DEBUG3 = 7;
  SEVERITY_NUMBER_DEBUG4 = 8;
  SEVERITY_NUMBER_INFO   = 9;
  SEVERITY_NUMBER_INFO2  = 10;
  SEVERITY_NUMBER_INFO3  = 11;
  SEVERITY_NUMBER_INFO4  = 12;
  SEVERITY_NUMBER_WARN   = 13;
  SEVERITY_NUMBER_WARN2  = 14;
  SEVERITY_NUMBER_WARN3  = 15;
  SEVERITY_NUMBER_WARN4  = 16;
  SEVERITY_NUMBER_ERROR  = 17;
  SEVERITY_NUMBER_ERROR2 = 18;
  SEVERITY_NUMBER_ERROR3 = 19;
  SEVERITY_NUMBER_ERROR4 = 20;
  SEVERITY_NUMBER_FATAL  = 21;
  SEVERITY_NUMBER_FATAL2 = 22;
  SEVERITY_NUMBER_FATAL3 = 23;
  SEVERITY_NUMBER_FATAL4 = 24;
}

enum LogRecordFlags {
  LOG_RECORD_FLAG_UNSPECIFIED = 0;
  LOG_RECORD_FLAG_TRACE_FLAGS_MASK = 0x000000FF;
}

Out of the 6 public enums 4 follow the same convention to prefix the value with the name of the enum to avoid conflicts (proto require that enum values are unique within a package, c++ requirement).

Luckily the 2 that do not follow are actually helper enums (not used in the real proto, not that it matters except for JSON format).

Proposal

We should follow the same convention, and probably document the convention for all enum values.

For LogRecordFlags (or LogRecordFlags -> LogRecordFlag): LOG_RECORD_FLAG_UNSPECIFIED -> LOG_RECORD_FLAG[S]_UNSPECIFIED LOG_RECORD_FLAG_TRACE_FLAGS_MASK -> LOG_RECORD_FLAG[S]_TRACE_FLAGS_MASK

For DataPointFlags: FLAG_NONE -> DATA_POINT_FLAGS_NONE FLAG_NO_RECORDED_VALUE -> DATA_POINT_FLAGS_NO_RECORDED_VALUE

Open Question

1. Is LogRecordFlags needed? If needed this most likely need to be in a common package since traceid/spanid are used in multiple places and probably flags will be added there as well.

Extra Problem

Especially on the tracing side, the enums are embedded into messages like Span {SpanKind} which makes generated code look a bit strange (e.g. Span_SpanKindClient).

Moving them to the main package instead of embedding them in the message is JSON encoding and protobuf encoding backwards compatible. We probably should consider that as well as part of the cleanup.

[bogdandrutu]

/cc @jmacd - as you added the DataPointFlags /cc @tigrannajaryan - as you added the LogRecordFlags

[dyladan]

Renaming LogRecordFlags and DataPointFlags is fine for JS. We don't export logs yet and our (outdated) metric exporter appears not to use DataPointFlags

[bogdandrutu]

@dyladan and as pointed in the description, the rename of the flag values is protobuf binary and JSON encoding backwards compatible since they are never on the wire, only used as helpers for producer/consumer to look into the int32 flags fields in the messages.

[dyladan]

ah sorry i didn't read closely enough. the generated code problem is also not affecting us right now, but we were planning to move to generated code for the OTLP exporters soon if possible, but it won't be publicly accessible. Just an internal detail.

[tigrannajaryan]

LogRecordFlags -> LogRecordFlag

Arguably this is fine. Flags can be plural to indicate this is a bit mask.

If we go with this logic then the only change needed is to add DATA_POINT_ prefix to DataPointFlags enum members.

[tigrannajaryan]

1. Is LogRecordFlags needed? If needed this most likely need to be in a common package since traceid/spanid are used in multiple places and probably flags will be added there as well.

I think it is a good way to pack small fields reasonably. I don't think we want to move it to common since we may want to add other flags which are only applicable to LogRecord.

[tigrannajaryan]

This is the only remaining issue that prevents declaration of OTLP/JSON stable. I tried 2 different PR but wasn't able to resolve this, so we need someone else to take this.

[tigrannajaryan]

This needs to be resolved for 1.0. I don't know how to solve it. Help is needed.

[bogdandrutu]

@tigrannajaryan as simple as removing the helpers for 1.0 :)) that solves 2 issues in the same time :)

[jmacd]

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

[tigrannajaryan]

Discussed in TC meeting and decided that https://github.com/open-telemetry/opentelemetry-proto/pull/461 should be sufficient and we can close this issue.