Proofread Logs Bridge API and SDK in preparation to mark it Stable

[tigrannajaryan]

We are preparing to mark Logs Bridge API/SDK Stable. Before we do that we need to proofread it, ideally by more than one person.

Anyone who is willing to work on this is welcome to become an assignee of this issue and file corrections/PRs.

[tigrannajaryan]

@open-telemetry/specs-logs-approvers FYI.

[martinkuba]

I spent some time going over the API and SDK spec, and it is not clear to me exactly how the implicit context injection should work. Since the include_trace_context config is for the Logger, I would assume that it would be the Logger's responsibility to add the span context fields on every LogRecord it emits. But there is also the Context parameter in the OnEmit method of the LogRecordProcessor, which does mention the include_trace_context option.

Questions

• Is it intended that the span context fields are set in the Logger itself or some processor?
• If it is the logger, what is the intent of passing the Context to the processor, and why does it rely on the include_trace_context config?

Also, there is a pending TODO in the Implicit Context Injection section.

[martinkuba]

The spec says that the Logger method for emitting log records may be named logRecord. Is this the recommended/preferable name over emit()?

[jack-berg]

Is it intended that the span context fields are set in the Logger itself or some processor?

No. Span context should either be set by the Logger (implicit context injection when include_trace_context=true), or explicitly by the caller if implicit context injection is not available of if include_trace_context=false.

If it is the logger, what is the intent of passing the Context to the processor, and why does it rely on the include_trace_context config?

The context that is available to processors may contain more than just trace context, for example baggage or any other entries in context.

Also, there is a pending TODO in the Implicit Context Injection section.

IMO opinion that TODO is acceptable because its in the "usage" section and doesn't contain normative language.

[martinkuba]

@jack-berg Thanks for the clarification. Since the Context parameter for the processors is for more than the trace context, why does the spec say that it should be empty when include_trace_context=false?

[jack-berg]

Right so you're referring to the context argument of LogRecordProcessor#onEmit, which says:

the Context that the SDK determined (the explicitly passed Context, the current Context, or an empty Context if the Logger was obtained with include_trace_context=false)

It is a bit strange that include_trace_context has side effects wider than just the trace context which is included on the log record. Maybe we should change the language to drop say:

the Context that the SDK determined (the explicitly passed Context or the current Context)

@open-telemetry/specs-logs-approvers WDYT?

[jack-berg]

#3354 also contributed to this.