Resolve log SDK context inconsistencies

[jack-berg]

Related to #3268.

The log SDK is inconsistent with Context: LogRecordProcessor#onEmit's accepts context and ReadWriteLogRecord as arguments, yet ReadWriteLogRecord provides access to Context, making the context argument appear unnecessary. Additionally, ReadableLogRecord says receivers have access to Context.

This is incorrect. It's important that ReadWriteLogRecord and ReadableLogRecord only have access to TraceContext, not the full Context, since we need to release references to the full Context ASAP to allow GC. This was the original intent behind making context an argument in LogRecordProcessor#onEmit rather than relying on accessing it via ReadWriteLogRecord.

This PR resolves this inconsistency by clarifying that ReadWriteLogRecord and ReadableLogRecord only can access TraceContext.

[jack-berg]

@CodeBlanch / @martinkuba we also chatted about whether this argument should be renamed to parentContext to match the trace SDK. I do not think we should do this. Tracing needs to differentiate between the parent context and the current span's context. Logs do not. The log context is just the context in which the record was recorded.

There's a similar concept in metrics where ExemplarFilter and ExemplarReservoir receive context arguments. The arguments are similarly called context, not parentContext, because there's no concept of parent in either logs or metrics.

[jack-berg]

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

[alanwest]

Why is Context listed as a parameter of a LogRecord in the bridge API spec here? The LogRecord data model does not have a Context Field. It only has fields relevant to trace context (trace/span id and flags).

The API emits LogRecords using the LogRecord data model.

A function receiving this as an argument MUST be able to set the following parameters:

• Timestamp
• Observed Timestamp
• Context that contains the TraceContext
• ...

Shouldn't this just be

The API emits LogRecords using the LogRecord data model.

A function receiving this as an argument MUST be able to set the following parameters:

• Timestamp
• Observed Timestamp
• TraceContext
• ...

If this were the case, then it seems we would not require the clarification this PR is posing.

[martinkuba]

My understanding is that having the Context on the LogRecord is the mechanism to explicitly provide a Context per LogRecord to processors' OnEmit function.

[alanwest]

That seems to differ from what has done with both traces and metrics. Take the Go SDK for example - which requires explicitly passing around context:

• Example from the trace API: the tracer.Start API accepts context as a parameter.
• Example from the metric API: the counter.Add API accepts context as a parameter.

I can only imagine that Go's eventual log bridge API would have an logger.Emit(ctx Context, log LogRecord) method also explicitly taking a context. Calling on @MrAlias, because like really... who am I to be attempting to represent Go? 😆

Speaking for .NET, though, the current context is simply available via ambient state, so we've no need to explicitly pass it around either on the LogRecord or as a parameter. This is what we've used with traces and metrics.

[jack-berg]

I think that splitting out context into a separate argument for onEmit also makes sense, and avoids the need for the changes this PR makes to ReadWriteLogRecord and ReadableLogRecord.

@alanwest care to open up a PR with that suggestion? If folks prefer that solution, we can close this and move forward.

[tigrannajaryan]

@alanwest here is the Go draft prototype. Context is indeed the first parameter of Emit().

However Emit does not accept a LogRecord, but rather an option list. This mimics the Go Trace API. So, you could say that this is the idiomatic way to pass a context-associated LogRecord in Go.

[tigrannajaryan]

One possible way to alter the Emit function description is to say that it may accept a LogRecord directly as a parameter or to accept the same information in some other form that is more idiomatic to the language (to allow what Go does).

[tigrannajaryan]

Here is another reference point, the Start Span function. All possible parameters (including the Context) are listed individually. I don't know if we want to to change Emit to also look like this in the spec (i.e. disaggregate LogRecord into separately listed parameters).

[jack-berg]

I don't know if we want to to change Emit to also look like this in the spec (i.e. disaggregate LogRecord into separately listed parameters).

I think that's a decision individual languages can / should make. In java, we commonly use the builder pattern across tracing and metrics, and it makes sense for us to do the same for logs. The emit API ends up looking like:

logger
  .logRecordBuilder()
  .setSeverity(Severity.INFO)
  .setBody("A log message from a custom appender without a span")
  .setAttribute(AttributeKey.stringKey("key"), "value")
  .setContext(...)
  .emit()

Context is just another setter alongside the other LogRecord fields. In my read, this is conformant with the current version of the spec, and would also be conformant to a version where Context is a separate parameter.

[tigrannajaryan]

@jack-berg I agree with you. I think some of the confusion comes from the fact that in the spec we list LogRecord as just a single parameter and then there seems to be an expectation that this should be translated to a language API literally, and Emit() should become a function call with just one parameter. But we don't really do this neither in Java, nor in Go. The SDK developers translate the spec's English prose into idiomatic API designs and as part of that translation they can choose to e.g. disaggregate parameters logically spec-ed as one parameter into separate parameters.

[alanwest]

Submitted #3354 as an alternative to this PR. It attempts to clarify that the spec does not require languages to literally have context as a parameter to the Emit() function.

[jack-berg]

Closing in favor of #3354.