Log record mutations are visible in next registered processors

[pellared]

What are you trying to achieve?

Define if log record mutations done in record processor are visible in next registered processors.

From #4010 (comment):

I believe the spec is not clear enough and can be interpreted in different ways leading to very different implementations (e.g. chained processors that can mutate data and see mutations of previous processor or parallel processors that don't see mutations of other processors).

We likely need to fix the spec. It is in a stable doc but is vague enough that I think it is a bug and should be fixed.

Since the spec is vague I am also worried that existing implementations in different language SDKs may have interpreted it differently and have made different choices. IMO, we need to start with auditing existing implementations and see where we stand currently and then decide how exactly we want to fix the spec (one of the options is that both "chained-allow-mutation" and "fanout-copy-to-parallel" should be possible like they are in the Collector pipelines).

Additional context.

• Issue originates from Logs: Allow the registered processor to be an independent pipeline #4010
• Add isolating log record processor #4062 (comment)

The undocumented assumption was the log processor will follow the pattern that mutations will be visible to the next processor to be consistent with span processors. However, the C++ Logs SDK is not designed this way (log records are passed as a copy to subsequent registered processors). Other Logs SDKs follow the pattern or are not stable.

[pellared]

I will check if it is possible to follow the pattern in Go even though it's pitfalls I outlined #4010 (comment). My main goal is to keep a possibility to have zero allocations when the log records are not modified (as this the the usual use case).

If we succeed to find a satisfying design for Go (and Rust), we could update the specification to make this requirement explicit.
However, what are we going to do with the fact that the stable C++ Logs SDK already has taken a different route? Would it have to be a "SHOULD" instead of a "MUST"?

CC @open-telemetry/rust-maintainers @open-telemetry/cpp-maintainers @open-telemetry/technical-committee

[jack-berg]

If a log processor's mutations were not visible to the next processor, then there would be no (or very little) point of onEmit receiving a ReadWriteLogRecord. If mutations are only visible to the processor being called, then the SDK might as well call onEmit with a ReadableLogRecord, since that processor can easily mutate with copy-on-write semantics.

Other evidence that the "mutations are not visible to subsequent processors" is the wrong interpretation:

• The log SDK was modeled after the trace SDK, and SpanProcessor#onStart modifications are visible to subsequent processors. It would be a very strange design if they weren't. .
• "LogRecordProcessors are invoked in the same order they were registered" (see spec). There's no point of this if mutations aren't visible to subsequent processors.
• "A LogRecordProcessor may freely modify logRecord for the duration of the OnEmit call. If logRecord is needed after OnEmit returns (i.e. for asynchronous processing) only reads are permitted" (see spec). The intent of this line is clarify that log processors can only mutate log records synchronously, since if they hold onto the record and mutate asynchronously, the mutation may conflict with one from a subsequent processor. See the Resolve lingering question of LogRecord mutability in LogRecordProcessor #2955 for more context, which was resolved by this statement.

However, what are we going to do with the fact that the stable C++ Logs SDK already has taken a different route?

Wondering if you can elaborate on this. I can't seem to find the LogRecordProcessor interface definition in c++ (I don't know c++ so I'm very naive navigating the code). Do log record processors call subsequent processors in c++? Is it possible to write a log processor which extracts and includes baggage attributes on log records in c++?

[pellared]

If a log processor's mutations were not visible to the next processor, then there would be no (or very little) point of onEmit receiving a ReadWriteLogRecord.

The modified record can be passed to a decorated processor. This is how mutations can be currently done in C++, Rust, and Go. E.g. mutatingProcessor found here: #4010 (comment)

Notice that currently there is nothing in the spec that allows creating a new/custom ReadableLogRecord.

Wondering if you can elaborate on this.

Sure. Regarding C++ my information is based on #4010 (comment).

[jack-berg]

The modified record can be passed to a decorated processor. This is how mutations can be currently done in C++, Rust, and Go.

There's no mention of a decorated processor in the spec, so it sounds like C++, Rust and Go invented new concepts. To be clear, I support an effort to evolve the spec to include accommodations which allow for higher performance implementations. However, I feel strongly that the current language of the spec does imply that log record mutations are visible to the subsequent processors. The design juggles several priorities, including performance and conceptual symmetry with prior art in otel (the trace signal in particular). A design featuring chaining or decorating processors might pave a clear path for a high performance implementation, it does so at the expense of conceptual symmetry.

My rust, c++, and go skills are pretty poor, but there may be creative language-specific ways to keep the door open to zero allocations while also staying within the spirit of the spec. For example, a processor might have a method indicating whether or not it mutates data. If none of the processors mutate data, or if there is only one processor registered, then the SDK could use an alternative implementation of ReadWriteLogRecord which avoids allocations.

[pellared]

Actually, it may occur that it won't be a problem for Go. I am in progress of evaluating it. I remember from my previous prototypes for most cases the Go escape analysis was doing good job. Reference /~https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#passing-record-as-pointer-to-loggeremit

Rust Logs SDK is also not stable and there was a discussion whether to use the same pattern as for span processors. See #4010 (comment)

It may be a good compromise to follow span processor pattern to achieve better consistency across languages as well as with tracing.