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

Stabilize trace context in non-OTLP formats and define casing #3909

Merged

Conversation

dashpole
Copy link
Contributor

Closes #3303

Changes

From @tigrannajaryan in #3303 (comment):

I think the conclusion in #3518 was that we keep the names. It has been a while and there is no new evidence that we need to do something else so I suggest that we submit a PR that declares https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/logging_trace_context.md "Stable" and closes this issue.

This marks the specification for using trace_id, span_id, and trace_flags in non-OTLP logging formats stable.

@jack-berg

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Let's keep this open for a while to ensure visibility.

@dashpole
Copy link
Contributor Author

dashpole commented Mar 5, 2024

@jmacd raised one potential concern: We want to add some OpenTracing fields (sampling priority?) to this specification, which have dots in them. This would potentially leave us with some fields with dots, and others with underscores. Josh, if there are related issues/PRs, feel free to add them here.

@tigrannajaryan
Copy link
Member

@jmacd raised one potential concern: We want to add some OpenTracing fields (sampling priority?) to this specification, which have dots in them. This would potentially leave us with some fields with dots, and others with underscores. Josh, if there are related issues/PRs, feel free to add them here.

Can you clarify which OpenTracing fields? Dots are totally fine at namespace separators. We use underscores as word separators.

@jmacd
Copy link
Contributor

jmacd commented Mar 6, 2024

I have a couple of concern about the absence of an explicit statement about tracestate, which is usually mentioned in the list of W3C trace context fields. In this case, I think it's OK to explicitly state that tracestate is not for recording in logs. We should, instead (IMO),define semantic conventions in case there are any useful fields in the tracestate that make sense for logs.

Here's what that looks like, for example: open-telemetry/semantic-conventions#793

In this case, I have work-in-progress to perform trace sampling based on OTEP 235 (https://github.com/open-telemetry/oteps/blob/main/text/trace/0235-sampling-threshold-in-trace-state.md) inside the collector-contrib probabilistic sampler processor, which applies to both spans and logs. What this means is that the sampler for logs will not try to encode sampling probability using an equivalent to tracestate in the log record, it will do that using attributes for randomness and threshold instead.

I propose this change before stabilizing: #3923

@jmacd
Copy link
Contributor

jmacd commented Mar 6, 2024

Related: #3924

@carlosalberto
Copy link
Contributor

@jmacd will #3924 will solve your concern? Can we go ahead and merge this PR or should we wait a little longer?

@jmacd
Copy link
Contributor

jmacd commented Mar 21, 2024

@carlos I thought we should merge #3923 before merging this one. #3924 is related but not a prereq

@jmacd
Copy link
Contributor

jmacd commented Mar 25, 2024

@carlosalberto please see #3923 (comment), thanks!

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
@jsuereth jsuereth removed the Stale label Apr 2, 2024
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 Apr 10, 2024
Copy link

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

@github-actions github-actions bot closed this Apr 18, 2024
@pellared
Copy link
Member

@open-telemetry/technical-committee, is this blocked by anything?

@arminru arminru removed the Stale label Jun 5, 2024
@arminru arminru reopened this Jun 5, 2024
Copy link

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

@carlosalberto
Copy link
Contributor

Ping @jsuereth @arminru @bogdandrutu @tigrannajaryan @jack-berg @pellared (who has previously approved this PR).

@carlosalberto
Copy link
Contributor

@dashpole It seems only minor edits are needed before we can (finally) merge this ;)

@dashpole dashpole force-pushed the stabilize_log_tracecontext branch from b2a9725 to 7a9de42 Compare July 9, 2024 14:23
@arminru arminru changed the title Stabilize the trace context in non-OTLP formats spec Stabilize trace context in non-OTLP formats and define casing Jul 10, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
dashpole and others added 2 commits July 10, 2024 10:06
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
@carlosalberto carlosalberto merged commit 9d4acd9 into open-telemetry:main Jul 12, 2024
6 checks passed
@dashpole dashpole deleted the stabilize_log_tracecontext branch July 12, 2024 12:02
@carlosalberto carlosalberto mentioned this pull request Jul 12, 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.

Consider adopting ECS convention for logging spec's Trace Context in Legacy Formats