Add SimpleConcurrentProcessor for Logs, clarify existing export concurrency

[cijothomas]

Fixes most of #4134 (Tracing signal not addressed in this PR)

Changes

1. Clarifies explicitly that the existing built-in processors should not invoke Export() concurrently. This was already the intention (from what I gather!), but not explicitly listed.
2. Adds SimpleConcurrentProcessor (happy to get alternative name suggestions), which can call Export() concurrently.

.NET, Rust, C++ have their SimpleProcessor already matching this.
Java, Go, Python does not. I believe they can fix the SimpleProcessor implementation to match this. This should be treated as a bug fix. Since SimpleProcessors were meant to be used in test/debug scenarios (stdout exporters), the extra perf hit should not be a concern. If it is indeed a concern, then users can be advised to switch to SimpleConcurrentProcessor.

(I also considered the possibility of adding extra capability to existing SimpleProcessor, but I believe it is better to have a dedicated one to avoid any confusion about "Simple" being in used for high-perf, prod scenarios. Also, adding more capabilities may make SimpleProcessor more complex without much gains.)

Additional background/context:
.NET, Rust, C++ had the equivalent of SimpleConcurrentProcessors for quite a while and was used when exporting to OS native tracing systems like ETW (Windows), Linux user_events. Exporting to these systems are done for scenarios requiring very high performance, and the existing Simple/Batch processors does not allow such high performance, necessitating the need of an official exporting processor.

(Note: A similar PR can be done for tracing sdk as well, but want to first try out in Log SDK. Once this is done, will make a follow up PR to trace sdk as well.)

• Related issues #
• Related OTEP(s) #
• Links to the prototypes (when adding or changing features)
• CHANGELOG.md file updated for non-trivial changes
• spec-compliance-matrix.md updated if necessary

[pellared]

I strongly suggest NOT using the term "concurrent" as usually it conveys that there is some of synchronization behind. E.g. "concurrent collections" are collections that can be used in multithreaded code. In some languages there is an idiom that if the type has Concurrent in its name, the method calls are synchronized.

My naming proposal is "Passthrough processor"

[jmacd]

This resonates with me too -- the use of "concurrent" suggests some kind of attention to concurrency, a limit of some sort in addition to offering concurrency. Maybe the name could be AsynchronousProcessor -- starts the export and returns success immediately.

I admit this worries me a bit -- is the exporter expected to implement some kind of limit, to avoid running out of memory? Can this be composed with the BatchSpanProcessor? What will control whether the BatchSpanProcessor drops spans vs. this processor consuming unlimited amounts of data?

Here, we have an OTel Collector processor that offers unlimited concurrency, but subject to a limit on the total amount of pending data, I consider it a potential solution to the problems posed above: https://github.com/open-telemetry/otel-arrow/tree/main/collector/processor/concurrentbatchprocessor

[cijothomas]

Maybe the name could be AsynchronousProcessor -- starts the export and returns success immediately.

That is not the intend. The intended exporters here not just 'starts' the export. It does everything inline (i.e serialization, writing to destination.), and then return success/failure. (from what I know, such exporters are typically writing to ETW, user-events)

[cijothomas]

is the exporter expected to implement some kind of limit, to avoid running out of memory?

No. Such exporters (etw/user-events) does not buffer anything. The logRecord is serialized and handed over to destination inline. ETW/user-events system is backed by OperatingSystem kernel memory, and they have ample mechanisms to keep memory in control, but those are outside the scope of the exporter.

[pellared]

@cijothomas any thoughts about my naming proposal?

My other proposals are

• "Direct processor" meaning that it directly calls the exporter.

• "Adapter processor" meaning that it is only an adapter and has no logic/ synchronization

[cijothomas]

naming is hard 🤣 I am not sure if any of the alternate suggestions are significantly better. I'll keep thinking and hope we get more suggestions.

[pellared]

👍

[pellared]

1. We cannot force custom implementation on how they document their implementations
2. It is not only about exporting but also ForceFlush and Shutdown

Suggested change

	implementation MUST document whether it supports concurrent `Export` calls, and
	implementation provided by the SDK MUST document whether it is concurrent safe, and

[cijothomas]

We cannot enforce, but the spec wording is to make sure any person authoring own exporter should follow this spec. If they don't follow, then there could be undesired behavior.
Alternatively, we can word it in such a way that "It must be documented to the exporter authors that they should document the exporters' concurrency characteristics....

[tsloughter]

I believe this was part of why it was went with that the processor should called all exporters the same and not have to worry about concurrency.

[pellared]

@tsloughter, the users could still use processors that does not require the exporters to be concurrent safe. But we would give a more performant option for cases where exporter are concurrent safe and can be used synchronously.

@cijothomas, the alternative could be that such exporter packages (like ETW or user_events exporters) would provide their own processors which are "tailored" to their exporters. It can give more flexibility and not increase the SDK functionality surface.

[cijothomas]

the alternative could be that such exporter packages (like ETW or user_events exporters) would provide their own processors which are "tailored" to their exporters

You are right! That is exactly what we have been doing for years. But that does not mean spec shouldn't support them.
The increase in SDK functionality surface is necessary, to support such scenarios. The spec already has wording that state implementations must have simple and batch (meaning others are optional), so additional processors don't put extra burden on implementations, unless they chose to support.

[pellared]

What is more (I have forgotten tot call it out explictly) I think that e.g. for ETW we can simply provide a single EtwProcessor which does the exporting. There is no need for batching or synchronization so there is no need to provide an Exporter interface implementation.

[cijothomas]

I think that e.g. for ETW we can simply provide a single EtwProcessor which does the exporting

Yes that is also possible. (In certain ways, OTel Rust does that. Its etwexporter is not really an exporter, but a ~processor).
But I think it is best to model exporters (the thing which does serialization, export telemetry to outside the process) as exporters itself consistently.

[pellared]

But I think it is best to model exporters

I am not really sure as these exporters are emitting batches. And for use cases like ETW and user_events you probably prefer to operate on "single" log record.

[cijothomas]

They are exporters! (They do the job of serializing and transferring the telemetry to an external entity.) Whether it gets a batch of 1 item or multiple is purely based on choice of processor used. (when used with SimpleProcessor, even OTLPExporter gets a batch of single item only).

[tsloughter]

Deleted my last comment because I had misread what was and wasn't in the current spec for Logs and don't want to confuse anyone reading these later.

Exporting can be done concurrently, what is the reason for needing a "ConcurrentProcessor"?

[cijothomas]

@tsloughter

Exporting can be done concurrently, what is the reason for needing a "ConcurrentProcessor"?

I don't think it is possible to achieve what we want today. We need exporter.export() method called synchronously from the processor. More specifically, when logger.logsomething("hello", foo=bar); statement returns, the LogRecord for that is already sent to processor, and to exporter and serialized and exported! Put differently, if the program crashes the moment after logsomething() returns, the user can be assured that no telemetry is lost due to being stuck in in-memory buffers.

[tsloughter]

@cijothomas right, and it is called synchronously. It is the exporter that handles concurrency, or lack there of. By default SimpleProcessor and builtin exporter must not return from export for spans and logs in order to support cases like you describe and it is relied on by users of like aws lambda, but it remains the exporter where concurrency is handled, not the processor.

[tsloughter]

Talking on Slack I now see what SimpleConcurrentProcessor is meant to achieve and will give here my description of it in case anyone else messed up like me and jumped to the conclusion it was meant to achieve the already hashed out "concurrent export" functionality:

The issue arises when you have N processes/threads each calling logger.log(record) concurrently and not wanting to block on any others data being sent but does want to block on their record being sent.

To repeat here what I said on slack, I would not do this with a SimpleConcurrentProcessor. I want to look at the existing examples given of exporters before I describe what I'd do in Erlang since it may just be more confusing than useful, but it would keep the restriction on Export intact and not add a SimpleConcurrentProcessor -- as well as have the ability to build on top a way to alleviate lock contention between threads on use of the SimpleProcessor through a SimpleProcessorPool.

[cijothomas]

I want to look at the existing examples given of exporters before I describe what I'd do in Erlang

OTel .NET:
Processor: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Exporter.Geneva/Internal/ReentrantExportProcessor.cs#L31-L34
Exporter: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Exporter.Geneva/MsgPackExporter/MsgPackLogExporter.cs#L116

OTel Rust:
Processor: https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/main/opentelemetry-etw-logs/src/logs/reentrant_logprocessor.rs#L35-L37
Exporter : https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/main/opentelemetry-etw-logs/src/logs/exporter.rs#L356

[cijothomas]

alleviate lock contention between threads on use of the SimpleProcessor through a SimpleProcessorPool.

I am not sure what is SimpleProcessorPool.

[jmacd]

Relates to #3616

[pellared]

My own previous suggestion:

It is not only about exporting but also ForceFlush and Shutdown

I double-checked and this would be a a breaking change in the specification.

Right now, it is allowed to call Shutdown or ForceFlush in parallel with Export. However, calls to Export has to be synchronized.

I created #4173 based on this PR to separate clarification of the export concurrency from proposing a new exporting processor.

[github-actions]

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

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.