span exporter concurrency: expand on Export() requirements

[tsloughter]

Fixes #2434

Changes

Additional context around what is and what isn't required of the Span Exporter Export() calls.

[trask]

Fixes #2434

Thanks!

For this to fully fix #2434, at least for Java, we also need to clarify the meaning of the batch span processor's exportTimeoutMillis. Because currently Java's batch span processor interprets exportTimeoutMillis as the amount of time to wait for the "promise" returned by the span exporter. And since this has default value of 30 seconds, the batch span processor basically forces synchronous behavior even when using an underlying async/concurrent exporter.

Similar to retries and concurrency, it seems like timeout may also be better handled by the underlying exporter (e.g. OTLP exporter already has OTEL_EXPORTER_OTLP_TIMEOUT).

This is tricky water though since batch span processor is a stable component, so definitely looking forward to more opinions here.

[tsloughter]

@trask good point. I think the timeout still makes sense but just also needs more details. It keeps the batch processor from stalling completely due to a malfunctioning exporter.

I suppose it is the same person configuring the batch processor and the exporter so maybe it is overkill to not just require the exporter have a timeout..

[tsloughter]

@trask have you asked anyone else in the Java SIG about why in the spec matrix it says concurrent sending is supported?

[trask]

@trask have you asked anyone else in the Java SIG about why in the spec matrix it says concurrent sending is supported?

The Java OTLP exporters themselves supports concurrent sending (and return promises to track each export's completion). It's the batch span processor that is the problem and does the blocking before proceeding on to the next batch (because of its interpretation of exportTimeoutMillis).

[tsloughter]

The Java OTLP exporters themselves supports concurrent sending (and return promises to track each export's completion). It's the batch span processor that is the problem and does the blocking before proceeding on to the next batch (because of its interpretation of exportTimeoutMillis).

Aah, ok.

[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.

[tsloughter]

Ugh. Thought it would notify me again before closing since I updated it after it was marked stale...

And apparently you can't re-open PRs when closed by this bot?

Won't bother re-opening as another branch right away since there are still changes to be made but I still need more feedback to make those.

[carlosalberto]

@tsloughter Just reopened this ;)

[tsloughter]

@carlosalberto thanks!

[github-actions]

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

[tsloughter]

I will get this updated this week and try to get feedback on the updates related to back pressure.

[reyang]

@jmacd @trask would you review the latest proposal here? Thanks!

[trask]

@open-telemetry/java-maintainers if this is merged, would we change the OTLP exporter behaviors to match? (and going back in time, would we have changed export() to not return a promise?)

[trask]

@open-telemetry/java-maintainers if this is merged, would we change the OTLP exporter behaviors to match? (and going back in time, would we have changed export() to not return a promise?)

to clarify, I'm supportive of this PR, I think this is the key:

Concurrent requests and retry logic is the responsibility of the exporter. The
default SDK's Span Processors SHOULD NOT implement retry logic, as the required
logic is likely to depend heavily on the specific protocol and backend the spans
are being sent to.

and given that retry logic could even include storing to disk and retrying later, which would be problematic for returning a promise anyways

[lalitb]

Approving it just to vote for these changes, even though my approval doesn't count :)

[tsloughter]

Changelog updated.