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.

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 W3C-specified trace flags to v1 Span proto #503

Merged
merged 13 commits into from
Sep 14, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 4, 2023

As discussed in open-telemetry/opentelemetry-specification#3411 and #382, the OpenTelemetry Span object does not record the trace flags that were provided in its Trace Context.

For a tail sampler to sample spans using mechanism consistent with head sampling, the flags must be available in the protocol. W3C reserves 8 bits for these flags, of which two are defined. I've added 32-bit fields in the protocol for the span and the span link here.

@jmacd jmacd requested review from a team August 4, 2023 18:04
@jmacd jmacd requested review from kalyanaj and blumamir August 4, 2023 18:05
opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved
opentelemetry/proto/trace/v1/trace.proto Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

Isn't this a duplciate of #384?

@jmacd
Copy link
Contributor Author

jmacd commented Aug 15, 2023

Isn't this a duplciate of #384?

Oh, yes!

@jmacd jmacd requested a review from a team August 21, 2023 18:28
@jmacd
Copy link
Contributor Author

jmacd commented Aug 21, 2023

cc/ @blumamir I closed #384 in favor of this.

@carlosalberto
Copy link
Contributor

@Oberon00 We are planning to merge this soon. Please take a last look, etc ;)

Comment on lines 114 to 117
// MUST not assume that 24 most significant bits will be zero.
// When creating new spans, the most-significant 24-bits MUST be
// zero. To read the 8-bit W3C trace flag (use flags &
// SPAN_FLAGS_TRACE_FLAGS_MASK). [Optional].
Copy link
Member

@Oberon00 Oberon00 Sep 8, 2023

Choose a reason for hiding this comment

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

I think "when creating new spans" may still be confusing in the context of the protocol spec / a proto comment as it could be understood as "creating new span messages", especially since the remainder seems to assume that "new spans" have a 32 bit flags field. I could not find a shorter unambiguous way than this:

Suggested change
// MUST not assume that 24 most significant bits will be zero.
// When creating new spans, the most-significant 24-bits MUST be
// zero. To read the 8-bit W3C trace flag (use flags &
// SPAN_FLAGS_TRACE_FLAGS_MASK). [Optional].
// MUST not assume that 24 most significant bits will be zero.
// To read the 8-bit W3C trace flag, use `flags & SPAN_FLAGS_TRACE_FLAGS_MASK`.
//
// When creating span messages, if the message is logically forwarded from another source
// with an equivalent flags fields (i.e., usually another OTLP span message), the field SHOULD
// be copied as-is. If creating from a source that does not have an equivalent flags field
// (such as a runtime representation of an OpenTelemetry span), the high 24 bits MUST
// be set to zero.
//
// [Optional].

Copy link
Member

Choose a reason for hiding this comment

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

Also, there seems to be a typo in:

To read the 8-bit W3C trace flag (use flags & SPAN_FLAGS_TRACE_FLAGS_MASK).

I assumed it should mean:

To read the 8-bit W3C trace flag, use flags & SPAN_FLAGS_TRACE_FLAGS_MASK.

Copy link
Member

@Oberon00 Oberon00 Sep 15, 2023

Choose a reason for hiding this comment

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

@carlosalberto @jmacd I think we forgot to apply an analogous change to the flags field in the link object.

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto carlosalberto merged commit ac3242b into open-telemetry:main Sep 14, 2023
14 checks passed
@jmacd jmacd deleted the jmacd/trace_flags branch September 14, 2023 18:59
VinozzZ pushed a commit to VinozzZ/opentelemetry-proto that referenced this pull request Jun 21, 2024
VinozzZ pushed a commit to VinozzZ/opentelemetry-proto that referenced this pull request Jun 21, 2024
VinozzZ pushed a commit to VinozzZ/opentelemetry-proto that referenced this pull request Jun 21, 2024
VinozzZ pushed a commit to VinozzZ/opentelemetry-proto that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants