Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification on SimpleProcessor concurrency #4134

Closed
pellared opened this issue Jul 9, 2024 · 8 comments · Fixed by #4205
Closed

Clarification on SimpleProcessor concurrency #4134

pellared opened this issue Jul 9, 2024 · 8 comments · Fixed by #4205
Assignees
Labels
spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@pellared
Copy link
Member

pellared commented Jul 9, 2024

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:

@pellared pellared added spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory labels Jul 9, 2024
@pellared pellared changed the title Clarification on SimpleProcessor Concurrency Clarification on SimpleProcessor concurrency Jul 9, 2024
@pellared
Copy link
Member Author

pellared commented Jul 9, 2024

Kudos to @cijothomas for noticing the issue.

@cijothomas
Copy link
Member

cijothomas commented Jul 9, 2024

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
Copy link
Member

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
Copy link
Member

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

@tsloughter
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

jsuereth commented Aug 7, 2024

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.

@jsuereth jsuereth added triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned and removed triage:deciding:tc-inbox Needs attention from the TC in order to move forward labels Aug 7, 2024
@pellared
Copy link
Member Author

pellared commented Aug 7, 2024

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.

carlosalberto pushed a commit that referenced this issue Sep 13, 2024
Same as
#4173
for metrics SDK.

Towards
#4134
carlosalberto pushed a commit that referenced this issue Sep 30, 2024
@austinlparker austinlparker moved this to Spec - Closed in 🔭 Main Backlog Oct 22, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: Spec - Closed
6 participants