Remove helper masks, not part of the protocol #423
Conversation
Fixes open-telemetry#363 The reason is to keep the proto interface as minimal as possible, and the helper masks are just helpers, and not part of the protocol. They can be defined by the language SIGs in their usages, but don't need to be part of the proto stability. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu
force-pushed
the
rmhelpermasks
branch
from
76f26e8
to
e491417
Compare
There was a problem hiding this comment.
The definitions of the bits in the flags field is an essential part of OTLP protocol. In my opinion, having these definitions in the form helper enums is valuable and helps prevent implementation mistakes. Because of this I would prefer to keep the helper enums.
|
Yeah, I agree - these helper numbers help ensure there isn't language implementation drift. I would vote to keep them. |
|
May help, but not part of the protocol and guarantees. I am definitely not going to accept this as part of our guarantees of stability. |
|
How can |
100% agree
YES |
|
I wonder what other @open-telemetry/specs-approvers think about usefulness of these enums. My preference would be to keep them, but it is not a strong opinion. Also, we should get rid of the zero values: LOG_RECORD_FLAG_UNSPECIFIED and FLAG_NONE since they encourage poor practice. The correct usage is to always do bitwise AND with the right bit, never compare to the 0 value. |
|
I'm w/ @tigrannajaryan in that I think having them here is generally helpful and helps us avoid (obvious) breaking changes in their definition as well as gives folks "one pane of glass" to look at. |
This is a protocol not a "helper" library. I don't think these constants belong to the protocol, similar to a standard like w3c trace context, does not require/define/encourage a constant for |
| // 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; |
There was a problem hiding this comment.
I believe the idea is to remove it in favor of comments describing values that can be used in data point flags. I believe that is a mistake. Having an enumeration allows for clearly named consistent values and separate documentation of each potential flag.
|
I disagree with the effort to protect users from being foolish. I support the idea of removing zero-valued enums, but not removing non-zero values. The FLAG_NO_RECORDED_VALUE has disappeared, which I do not support. Users who use bitmasks should know what they are doing and/or read documents. |
|
Discussed in TC meeting and decided that #461 should be sufficient and we can close this issue. |
Fixes #363
The reason is to keep the proto interface as minimal as possible, and the helper masks are just helpers, and not part of the protocol. They can be defined by the language SIGs in their usages, but don't need to be part of the proto stability.
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com