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 LogRecordProcessor OnEmit context parameter #3387

Conversation

martinkuba
Copy link
Contributor

The context parameter passed in to LogRecordProcessor.OnEmit() should not be affected by the include_trace_context configuration, since the Context may contain other information than just the trace context.

@martinkuba martinkuba requested review from a team April 12, 2023 21:14
@martinkuba martinkuba changed the title Clarify OnEmit context parameter Clarify LogRecordProcessor OnEmit context parameter Apr 12, 2023
passed `Context`, the current `Context`, or an empty `Context` if
the [Logger](./bridge-api.md#get-a-logger) was obtained
with `include_trace_context=false`)
passed `Context` or the current `Context`)
Copy link
Member

Choose a reason for hiding this comment

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

Question: does OnEmit need to know the value of include_trace_context?

Copy link
Member

Choose a reason for hiding this comment

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

Related question: should a log's trace context fields be populated if include_trace_context=false and context is explicitly passed? I think yes. However, for languages that rely on explicit context because no implicit context is available, there's no point to having the include_trace_context parameter since it doesn't change any behavior.

We might actually consider removing include_trace_context, since setting it to true seems to only be syntactic sugar for explicitly setting an empty context.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we should consider it, it is a source of confusion. Created a separate issue here: #3394

@jack-berg
Copy link
Member

Superseded by #3397.

@jack-berg jack-berg closed this Apr 14, 2023
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.

3 participants