Skip to content

Clarify legacy logs format trace context flags and tracestate#3923

Closed
jmacd wants to merge 8 commits intoopen-telemetry:mainfrom
jmacd:jmacd/log_legacy
Closed

Clarify legacy logs format trace context flags and tracestate#3923
jmacd wants to merge 8 commits intoopen-telemetry:mainfrom
jmacd:jmacd/log_legacy

Conversation

@jmacd
Copy link
Copy Markdown
Contributor

@jmacd jmacd commented Mar 6, 2024

Part of #3303.

Changes

Clarifies how the trace flags are represented when placing trace context in log records.
Clarifies that tracestate is not encoded when placing trace context in log records.

Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with a changelog entry.

@jmacd jmacd requested a review from a team March 6, 2024 23:57
@github-actions
Copy link
Copy Markdown

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 15, 2024
@carlosalberto
Copy link
Copy Markdown
Contributor

@pellared maybe re-review this? Or still blocked?

@github-actions github-actions bot removed the Stale label Mar 16, 2024
Comment on lines +242 to +247
Description: Trace flags as defined in [W3C Trace Context Level
2](https://www.w3.org/TR/trace-context-2/#trace-flags)
specification. At the time of writing the specification defines two
flags - the SAMPLED flag and the RANDOM flag. This field is logical-OR
combined into the least-significant 8-bits of the `Flags` field. This
field is optional.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See ^^^ 4 lines up it says we have a field named TraceFlags. I'm clarifying that it's not real. It's conceptual, and it's actually encoded in the (undocumented here) field named Flags.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm clarifying that it's not real.

I am not sure I agree with that. It seems pretty real from data model perspective. In the data model we do have a TraceFlags field, as real as it gets in data models. The fact that OTLP uses a Flags field to encode this information is not important from the perspective of this document.

It seems we are bringing the implementation details of OTLP into the data model doc and I don't understand why.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My intention here was to clarify what may be internal confusion of mine -- I was recently surprised to learn how the W3C Trace Context defines propagation of unknown flags. Specifically, participants MUST zero the flags they do not recognize. I bring this up because the link on "as defined in W3C Trace Context" links to an overview section, whereas if you read https://www.w3.org/TR/trace-context/#other-flags you may question what it means to be "as defined in". It looks to me that if any flag besides the two we know about (Sampled, 0x1 and Random, 0x2) makes an invalid trace flags value, and perhaps the data model should say something about this.

However, my only intention was to be very clear that if TraceFlags arrives as a string like "ffff" we do not consider it valid TraceFlags, because it is too wide. I think we mean "syntactically defined as trace flags are defined" but that we are not forbidding legacy logs formats from creating logs data with unrecognized flags set as I speculated about above.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 2, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants