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

Clarify legacy logs format trace context flags and tracestate #3923

Closed
wants to merge 8 commits into from

Conversation

jmacd
Copy link
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
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
Copy link

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
Contributor

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

@github-actions github-actions bot removed the Stale label Mar 16, 2024
specification/logs/data-model.md Outdated Show resolved Hide resolved
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
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
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
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.

Copy link

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
Copy link

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