Ensure helper enums are consistently declared #405

tigrannajaryan · 2022-06-21T13:17:22Z

The changes in this PR do not affect wire formats in any way
since these are helper enums. This is not a breaking change
for the protocol.

This is a breaking change for the generated code.

This change is the smaller subset of #397

bogdandrutu · 2022-06-21T14:30:13Z

opentelemetry/proto/logs/v1/logs.proto

 // Masks for LogRecord.flags field.
 enum LogRecordFlags {
-  LOG_RECORD_FLAG_UNSPECIFIED = 0;
-  LOG_RECORD_FLAG_TRACE_FLAGS_MASK = 0x000000FF;
+  LOG_RECORD_FLAGS_UNSPECIFIED = 0;
+  LOG_RECORD_FLAGS_TRACE_FLAGS_MASK = 0x000000FF;
 }


I would rather remove them, since this repo is for the protocol definition and not for the "helpers" around the protocol.

These help to implement the protocol correctly. Without these enums we have to explain the flags using English prose, which is more fragile and error prone.

I understand the desire to minimize the contract surface but I believe to help increase OTLP popularity we need to take some pains ourselves in order to reduce the pains for others who adopt OTLP. See my comment about the proposed approach here: #400 (comment)

I disagree since a standard like w3c does not define "helpers" for trace flags for example. Also it is very bad design right now that helpers related to trace flags are defined in "logs" package.

I strongly suggest to remove for the moment, and determine if you are right or not about the "wrong" implementation, once we first hear about that we can have a small design for the helpers: where do they live, what are the guarantees since in this repo we mention that we guarantee wire not generated code (then how do you use this?), etc.

I disagree since a standard like w3c does not define "helpers" for trace flags for example.

I am not sure the analogy with w3c standard that you bring helps. The entire w3c trace context spec is just English prose, so there is no mechanism available for formal, compiler-enforced enum declarations there. I think the fact that w3c standard doesn't define helpers is not an argument that helps us to make a decision in our case, where such Protobuf compiler-enforced mechanism is luckily available.

Also it is very bad design right now that helpers related to trace flags are defined in "logs" package.

I think there is some confusion about what this enum is. This helper is not trace flags definitions. This helper enum is the definition of LogRecord.flags field. Yes, 8 bits of the flags field will contain the value of w3c trace flags, but note how we don't define what is contained in those 8 bits here - the fact that only 1 bit is actually used here is irrelevant here, is not OTLP's business and is intentionally omitted. There may be other LogRecord flag definitions added here in the future, totally unrelated to the w3c trace flags. So, this is really LogRecord.flags field definition. Can you clarify why you think it is a bad design?

To be clear, I am not strongly opposed to removing the helpers. I do think the helpers are useful and have a slight preference to keep them, but if you see strong arguments in favour of removing we can remove them.

I'm with @tigrannajaryan. Soft-yes to include them, mostly because I don't see strong arguments for removing them.

opentelemetry/proto/metrics/v1/metrics.proto

Aneurysm9 · 2022-06-21T15:17:48Z

opentelemetry/proto/metrics/v1/metrics.proto


  // 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;
+  DATA_POINTS_FLAG_NO_RECORDED_VALUE = 1;


Suggested change

DATA_POINTS_FLAG_NO_RECORDED_VALUE = 1;

DATA_POINTS_FLAGS_NO_RECORDED_VALUE = 1;

If we're going to change these we should be consistent with the LOG_RECORD_FLAGS_* values.

@tigrannajaryan my point on removing seems to be proven here. I DATA_POINTS_FLAGS_NO_RECORDED_VALUE sounds like the value of the "flags" not the value of the "mask" that you need to "bit and/or". I do suggest even more to remove them.

DATA_POINTS_FLAGS_NO_RECORDED_VALUE sounds like the value of the "flags" not the value of the "mask" that you need to "bit and/or". I do suggest even more to remove them.

I am not sure understand. Here is what the enum comment currently says:

// DataPointFlags is defined as a protobuf 'uint32' type and is to be used as a // bit-field representing 32 distinct boolean flags. Each flag defined in this // 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

So, it is a bitmask, right? DATA_POINTS_FLAGS_NO_RECORDED_VALUE is equal to 1, denoting the least significant bit in this field.

Please clarify where do you see a problem here that proves your point. Are you saying the name DATA_POINTS_FLAGS_NO_RECORDED_VALUE is confusing because it is not clear it is a mask?

opentelemetry/proto/metrics/v1/metrics.proto

The changes in this PR do not affect wire formats in any way since these are helper enums. This is not a breaking change for the protocol. This is a breaking change for the generated code. This change is the smaller subset of open-telemetry#397

jsuereth · 2022-07-08T17:43:22Z

opentelemetry/proto/metrics/v1/metrics.proto

 //
 enum DataPointFlags {
-  FLAG_NONE = 0;
+  DATA_POINTS_FLAGS_NONE = 0;


This name change seems 100% inline w/ C++ generated code (vs. other language). Is that correct? Do we have a naming convention for enums that requires the value name to include the enum name?

jsuereth · 2022-07-08T17:44:40Z

opentelemetry/proto/logs/v1/logs.proto

 // Masks for LogRecord.flags field.
 enum LogRecordFlags {
-  LOG_RECORD_FLAG_UNSPECIFIED = 0;
-  LOG_RECORD_FLAG_TRACE_FLAGS_MASK = 0x000000FF;
+  LOG_RECORD_FLAGS_UNSPECIFIED = 0;
+  LOG_RECORD_FLAGS_TRACE_FLAGS_MASK = 0x000000FF;
 }


I'm with @tigrannajaryan. Soft-yes to include them, mostly because I don't see strong arguments for removing them.

tigrannajaryan · 2022-07-27T22:09:59Z

Closing since there is no consensus on how to move forward.

tigrannajaryan requested review from a team and Aneurysm9 June 21, 2022 13:17

github-actions bot assigned jmacd Jun 21, 2022

tigrannajaryan mentioned this pull request Jun 21, 2022

Ensure enums are consistently declared #397

Closed

bogdandrutu reviewed Jun 21, 2022

View reviewed changes

tigrannajaryan mentioned this pull request Jun 21, 2022

Decide Conditions on Symbolic Stability to Declare OTLP 1.0 #400

Closed

Aneurysm9 reviewed Jun 21, 2022

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/helperenums branch from 02739bb to 9004b9d Compare June 21, 2022 22:12

tigrannajaryan force-pushed the feature/tigran/helperenums branch from 9004b9d to 401b3f8 Compare June 21, 2022 22:13

jsuereth approved these changes Jul 8, 2022

View reviewed changes

tigrannajaryan closed this Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure helper enums are consistently declared #405

Ensure helper enums are consistently declared #405

tigrannajaryan commented Jun 21, 2022

bogdandrutu Jun 21, 2022

tigrannajaryan Jun 21, 2022

bogdandrutu Jun 21, 2022

tigrannajaryan Jun 21, 2022

jsuereth Jul 8, 2022

Aneurysm9 Jun 21, 2022

bogdandrutu Jun 21, 2022

tigrannajaryan Jun 21, 2022

jsuereth Jul 8, 2022

jsuereth Jul 8, 2022

tigrannajaryan commented Jul 27, 2022

	DATA_POINTS_FLAG_NO_RECORDED_VALUE = 1;
	DATA_POINTS_FLAGS_NO_RECORDED_VALUE = 1;

Ensure helper enums are consistently declared #405

Ensure helper enums are consistently declared #405

Conversation

tigrannajaryan commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Jul 27, 2022