Skip to content

Clarify LogRecordProcessor OnEmit context parameter#3387

Closed
martinkuba wants to merge 1 commit intoopen-telemetry:mainfrom
martinkuba:clarify-log-processor-onemit-context-parameter
Closed

Clarify LogRecordProcessor OnEmit context parameter#3387
martinkuba wants to merge 1 commit intoopen-telemetry:mainfrom
martinkuba:clarify-log-processor-onemit-context-parameter

Conversation

@martinkuba
Copy link
Copy Markdown
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
Comment thread specification/logs/sdk.md
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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