Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing trace flags to Span and Link messages #384

Closed
wants to merge 1 commit into from
Closed

Add missing trace flags to Span and Link messages #384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions opentelemetry/proto/trace/v1/trace.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,15 @@ message Span {
// dropped_attributes_count is the number of dropped attributes. If the value is 0,
// then no attributes were dropped.
uint32 dropped_attributes_count = 5;

// 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

// 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;
Comment on lines +294 to +302
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this closer to the ids?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// links is a collection of Links, which are references from this span to a span
Expand All @@ -304,6 +313,16 @@ message Span {
// An optional final status for this span. Semantically when Status isn't set, it means
// span's status code is unset, i.e. assume STATUS_CODE_UNSET (code = 0).
Status status = 15;

// 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 = 16;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No empty line.

}

// The Status type defines a logical error model that is suitable for different
Expand Down