Add missing trace flags to Span and Link messages #384
Conversation
|
I now wonder if the otlp messages should also contain the w3c version so when more flags are added in the future, the receiver can differentiate between them being set to "0" vs "not set" by examining the version. |
|
|
||
| // Flags, a bit field. 8 least significant bits are the trace flags as | ||
| // defined in W3C Trace Context specification. 24 most significant bits are reserved | ||
| // and must be set to 0. Readers must not assume that 24 most significant bits | ||
| // will be zero and must correctly mask the bits when reading 8-bit trace flag (use | ||
| // flags & TRACE_FLAGS_MASK). | ||
| // This value is required, as not setting it will be interpeted as 0 which is | ||
| // a valid representation of all flags being present and set to 0. | ||
| fixed32 flags = 6; |
There was a problem hiding this comment.
Can you move this closer to the ids?
There was a problem hiding this comment.
No problem, but it looks like the messages are currently sorted by "field number", and moving it up will break this pattern. Are we OK with that?
|
@blumamir I think we can add the version when w3c changes to v2, before that, the absence of the version means version 1. |
If we add it when w3c bump the version, then all existing exporters out there will not report it which will be interpreted as It means that a processor will not be able to use a new flag until all the exporters in production are upgraded to a version that supports the new |
Isn't that true anyway? Let's assume we add the field and everyone just exports "v=1", what is the difference? |
| // defined in W3C Trace Context specification. 24 most significant bits are reserved | ||
| // and must be set to 0. Readers must not assume that 24 most significant bits | ||
| // will be zero and must correctly mask the bits when reading 8-bit trace flag (use | ||
| // flags & TRACE_FLAGS_MASK). |
There was a problem hiding this comment.
Where is TRACE_FLAGS_MASK definition?
We have LOG_RECORD_FLAG_TRACE_FLAGS_MASK that is defined elsewhere. Should the flags field here in the LogRecord have exactly the same bit masks? If yes then move and rename LOG_RECORD_FLAG_TRACE_FLAGS_MASK to common.proto. If no then define a separate enum here.
There was a problem hiding this comment.
Given that there are other proposals that may require adding more flags to Spans (but not to LogRecord) I think it is a good idea to have a separate SpanFlags enum that is defined independently from LogRecordFlags (even it uses the same bits for trace flags), e.g.:
// Masks for LogRecord.flags field.
enum SpanFlags {
SPAN_FLAG_UNSPECIFIED = 0;
SPAN_FLAG_TRACE_FLAGS_MASK = 0x000000FF;
}
|
Should we maybe wait a bit with this and do it together with IsRemote once open-telemetry/oteps#182 is merged, to minimize the chance of inconsistencies? |
We can wait. If it is delayed for a long time we can also move forward with this since I think we can add new flags in backward compatible manner. For IsRemote we likely will need 2 bits to represent (Unspecified=00, IsRemote=01, IsNotRemote=01) state. Unspecified will be the default state matching the zero state of the field (which will make it backward compatible with existing OTLP versions). |
|
open-telemetry/oteps#182 has not seen much progress. Do we want to move forward with this PR? |
| // This value is required, as not setting it will be interpeted as 0 which is | ||
| // a valid representation of all flags being present and set to 0. | ||
| fixed32 flags = 16; | ||
|
|
fixes #382 and #166