Add isolating log record processor

[pellared]

Why

Fixes #4010 (read the issue for more details)

The SDK SHOULD offer a way to make independent log processing. I find it as a missing feature that should be covered by by the specification.

What

This PR tries to achieve it in the following way:

• SDK SHOULD provide an isolating processor allowing independent log processing pipelines. Source: Logs: Allow the registered processor to be an independent pipeline #4010 (comment)

This approach should be solving the issue at least for Java, .NET, Python, JavaScript, Go. This proposal should not make any existing log SDKs not compliant with the specification.

I think that the same approach could be also adopted in tracing for span processors (but I have not yet investigated it in depth).

Remarks

I created a separate issue for specifying whether (by default) a change to a record made in a processor is propagated to next registered processor. See #4065

[pellared]

SIG meeting notes.
TODOs based on quick review during the meeting:

1. Ensure that there is no conflict with Add ability for SpanProcessor to mutate spans on end #4024
2. Expand the description (e.g. describe the reasons, alternatives) to make reviewing easier.

[yurishkuro]

High level feedback:

(1) I think so far the specification for the SDKs was far more opinionated than this PR, by describing not only capabilities but the desired architecture, i.e. standard components like processor, exporter, and standard ways to wire them together.

I think "multiple pipelines" is a significant capability change that requires a description of the architecture. E.g. it may require bifurcating the processing chain, so how would that be achieved? I don't think this is enough to just say "allow making a copy of log record".

(2) Whose responsibility should it be to decide on parallel pipelines? Should it be happening just because of the implementation of specific components? Or should it be user-driven decision by setting up the parallel pipelines accordingly?

(3) Does this capability only make sense to logs, or to other signals as well? Why or why not? I think we should be consistent.

[pellared]

Regarding the relation to #4024

I would say that #4024 is a "retrofit" as the original OnEnd does not allow mutation of the span when it occurs that it is needed in some cases. The OnEmit already allows mutation of the log records. I do not see how this PR is in conflict with #4024.

CC @jmacd

[pellared]

High level feedback:

👍

(1) I think so far the specification for the SDKs was far more opinionated than this PR, by describing not only capabilities but the desired architecture, i.e. standard components like processor, exporter, and standard ways to wire them together.

I think "multiple pipelines" is a significant capability change that requires a description of the architecture. E.g. it may require bifurcating the processing chain, so how would that be achieved? I don't think this is enough to just say "allow making a copy of log record".

The biggest problem here is that almost stable Logs SDKs have already different architectures (and different ways of processing logs). I am not sure if e.g. Java, JavaScript .NET would like to say e.g. "before making a change in a processor, make a copy so that other processors are not affected". E.g. in Go we have a comment like this: "Before modifying a Record, the implementation must use Record.Clone to create a copy that shares no state with the original".

(2) Whose responsibility should it be to decide on parallel pipelines? Should it be happening just because of the implementation of specific components? Or should it be user-driven decision by setting up the parallel pipelines accordingly?

I am not sure if I follow. I think it can be both of "because of the implementation of specific components" and "user-driven decision by setting up the parallel pipelines" at the same time 😉

(3) Does this capability only make sense to logs, or to other signals as well? Why or why not? I think we should be consistent.

I think it makes sense for all signals. I will look into this.

At last, I think that this PR needs more feedback from other SDK maintainers.

[jmacd]

@pellared and @JonasKunz I'm trying to bring together your two proposals. It seems after reading this and #4024, there is a desire for OTel SDKs to support automatic "fanout" processors. I think I would rather see us specify that SDKs should support an idiomatic fanout processor which hides details related to avoiding shared-mutable references, copy-on-write protections, and so on.

[CodeBlanch]

My thoughts and perspective from .NET...

Originally we had an immutable structure LogRecord. Users asked for it to become mutable so we did that. .NET has a chained processor pipeline so all processors are called in the order added and all see the same mutable LogRecord.

No one that I can recall has complained about that. But one request which has come up a bunch is the ability to filter.

ProcessorA(Export records matching ABC) -> ProcessorB(Export all records)

There is no good way to accomplish that currently in .NET.

I always envisioned some kind of fork processor might help with that. Thinking out loud and combining it with this idea to copy the LogRecord it could look like:

LogRecordA emitted
  -> ForkProcessor(for log records matching ABC, LogRecordB = LogRecordA.Copy()) -> Export(LogRecordB)
  -> Processor(add some data to LogRecordA)
  -> ForkProcessor(for all log records, LogRecordC = LogRecordA.Copy()) -> Processor(redact LogRecordC) -> Export(LogRecordC)
  -> Processor(add more data to LogRecordA)
  -> Export(LogRecordA)

PS: When I say "Copy" we do have a pool so it isn't necessarily an allocation.

[pellared]

@CodeBlanch

When I say "Copy" we do have a pool so it isn't necessarily an allocation.

I just want to share that making pools for log records may be "dangerous" as the user-defines processor/exporter may want to try to use the log records after OnEmit has returned (e.g. asynchronous processing, batching etc). I did my best to describe it here: https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#record-attributes-as-slice

[pellared]

I just want to add that even for implementation that does not "fully" share the log record between processors (see: #4067) this processor may still be handy. E.g. currently in Go most of the log record data is not shared. But there is an exception: log attributes. This is done for performance optimization to reduce the heap allocations (allows zero-heap allocation).

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[carlosalberto]

This has been open for a little while, and albeit many people are on holidays, we are safe as this is a Development addition. Merging.