Skip to content

Drop log API include_trace_context parameter#3397

Merged
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:drop-include-trace-context
Apr 14, 2023
Merged

Drop log API include_trace_context parameter#3397
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:drop-include-trace-context

Conversation

@jack-berg
Copy link
Copy Markdown
Member

Resolves #3394.

@jack-berg jack-berg requested review from a team April 13, 2023 20:33
@jack-berg
Copy link
Copy Markdown
Member Author

FYI @martinkuba. If this merges it would supersede #3387. WDYT?

@jack-berg
Copy link
Copy Markdown
Member Author

FYI @scheler I believe include_trace_context was originally added by you. See my arguments here for its removal. The same accomplished with include_trace_context=false is still possible with this change.

@martinkuba
Copy link
Copy Markdown
Contributor

FYI @scheler I believe include_trace_context was originally added by you. See my arguments here for its removal. The same accomplished with include_trace_context=false is still possible with this change.

@jack-berg If I am reading this change correctly, the trace context will always be populated (it says MUST). So, if I did not want the trace context populated, I would have to call logger.emit() with empty context parameter. But I think that means that processors would also get empty context? I don't think that's a side effect that we want.

With that said, I don't know if there is a use case for not wanting to set trace context.

@jack-berg
Copy link
Copy Markdown
Member Author

@martinkuba you can achieve the goal of having LogProcessor#onEmit(Context,LogRecord) have a useful context while LogRecord has empty trace context fields by setting explicit context when calling logger.emit, but first removing the trace context. In java, I'd accomplish this with Context.current().with(Span.getInvalid()).

@martinkuba
Copy link
Copy Markdown
Contributor

@martinkuba you can achieve the goal of having LogProcessor#onEmit(Context,LogRecord) have a useful context while LogRecord has empty trace context fields by setting explicit context when calling logger.emit, but first removing the trace context. In java, I'd accomplish this with Context.current().with(Span.getInvalid()).

I think that's a good workaround but more inconvenient than having a logger instance that does not add the trace context to any logs it emits. I am just pointing out the difference but do not feel strongly about keeping this configuration.

@scheler
Copy link
Copy Markdown
Contributor

scheler commented Apr 14, 2023

@jack-berg this is fine. When the logs api was introduced it did not list out the parameters for logger operations explicitly. With the section on implicit context injection, I felt the need for a setting to avoid setting context on the log record.

Now with the explicit Context parameter in logger.emit, is there still a need to have these two sections in the spec?

Implicit Context Injection

Explicit Context Injection

@tigrannajaryan
Copy link
Copy Markdown
Member

Now with the explicit Context parameter in logger.emit, is there still a need to have these two sections in the spec?

Implicit Context Injection

Explicit Context Injection

I think the sections are still necessary to explain how it is supposed to work. This PR makes the necessary changes in those sections.

@jack-berg
Copy link
Copy Markdown
Member Author

I think that's a good workaround but more inconvenient than having a logger instance that does not add the trace context to any logs it emits.

Luckily its log appenders that call this API so they only need to write the code that excludes trace context context in one place.

I think the sections are still necessary to explain how it is supposed to work. This PR makes the necessary changes in those sections.

I took another look at those sections and wasn't satisfied with how their language had diverged from language we use elsewhere in the document. I rephrased it for consistency.

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 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 removing include_trace_context from "Get a Logger" API

8 participants