Clarification on SimpleProcessor concurrency

[pellared]

The current specification states that "Export will never be called concurrently for the same exporter instance," but some exporters (e.g., ETW, user_events, LTTng) can handle concurrent exports. This leads to potential inefficiencies.

There is also discrepancy regarding the concurrency behavior of the SimpleProcessor across different OpenTelemetry language implementations (not all languages synchronize the calls to the exporter).

Proposed Solution:
Allow Exporter Concurrency: Update the specification to allow exporters to be used concurrently.

References:

• OTel Specification: Exporter Section
• OTel Go Implementation Discussion

[pellared]

Kudos to @cijothomas for noticing the issue.

[cijothomas]

Current state:
.NET, C++, Rust - SimpleProcessor synchronize calls to export(), so that only one export() is active at any point.
Java, Go, Python - SimpleProcessor does not synchronize calls to export(), allowing concurrent export().

[cijothomas]

Two sentences from the spec which states that only one export() will be called at a time.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#export

Export will never be called concurrently for the same exporter instance.

while an instance of an exporter will never have it Export called concurrently it does not mean that...

If exporters are authored with this assumption, then pairing them with SimpleProcessor may yield unexpected results. For example, if a stdout/console exporter assumes it'll only be called from one thread at a time, it does not have to lock the stdout while writing, like shown below.

stdout.write(log.Body);
stdout.write(log.Attribute[0]..[n);
stdout.werite(log.InstrumentationScope);
...

If SimpleProcessor calls such an exporter from different threads, then the output can get mixed up. (Trask confirmed that Java's stdout exporter makes a single call to write to console, so the output is not mixed up. But .NET consoleexporter makes multiple calls without syncronization, so unless the paired processor ensures synchronous calling, output gets mixed up!)

I think we should update the wording on Exporter spec, to allow concurrent Export() methods, and also modify the SimpleExportProcessor section to make it clear that they should export concurrently or synchronously, depending on the Exporter's advertised expectation. If exporter has no trouble being called concurrently, then SimpleProcessor can leverage that. Otherwise, SimpleProcessor must ensure only calls to export() are done one after another.

[cijothomas]

I self-assigned this. Will send a PR to clarify the wording here next week.

[tsloughter]

We've tried to clear this up before. I think it is my wording in there around what is to be done to support concurrency that may be confusing :)

The "never be called concurrently" applies to the Export method/function for a particular instance. Whether that method/function returns before or after the actual data is sent over the wire is up not specified by the specification. Meaning, Export could return some identifier for the particular export request while the actual exporting is ongoing and then a message of "success" or "fail" is sent at a later time to the processor to let it know if that export was a success/failure.

So Export must return but when/what it returns is left open to allow concurrency be handled th best way for each particular runtime.

So yes, exporting can happen concurrently, but no we can not allow Export to be called concurrently.

[cijothomas]

but no we can not allow Export to be called concurrently.

Why? Exporters like the ones to etw, user_events can not only be called concurrently, but they must be called that way to ensure highest perf with no contention.

[jsuereth]

TC Triage: There are general issues with concurrency assumptions that are not panning out in practice as intended. This is one of a set of issues where we need to improve our concurrency portions of the specification.

@jmacd is willing to sponsor this issue.

[pellared]

Side note: I created #4173 as I think that #4163 tries to accomplish to many concerns in a single PR. My PR may be closed if anyone finds that it brings more confusion.