Increase export throughput #2204
Conversation
This reverts commit b2c60e7.
| flushRequested.set(null); | ||
| CompletableResultCode.ofAll(pendingExports).join(exporterTimeoutNanos, TimeUnit.NANOSECONDS); | ||
| CompletableResultCode flushResult = flushRequested.get(); | ||
| if (flushResult != null) { |
There was a problem hiding this comment.
I pulled this change in from upstream
There was a problem hiding this comment.
maybe upstream needs to be updated? based on the old code, null check doesn't seem to be necessary.
There was a problem hiding this comment.
it looks like upstream added the null check to make for nullaway static analysis happy, I'd rather stay in sync with them
| pendingExports.remove(result); | ||
| }); | ||
| } else { | ||
| result.join(exporterTimeoutNanos, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
can we add log here to warn users that the maxPendingExports is reached here?
| pendingExports.add(result); | ||
| result.whenComplete( | ||
| () -> { | ||
| pendingExports.remove(result); | ||
| }); | ||
| } else { | ||
| addAsyncExport.recordFailure( |
There was a problem hiding this comment.
would be nice to track this via statsbeat?
can we add a todo? it's hard to go through the review with the whole team to create a new meter for this in StatsbeatNetwork.. but we can track it through StatsbeatFeature.
There was a problem hiding this comment.
I don't expect this to be common, it allows 100 concurrent exports
The first commit is reverting the previous implementation (which was a mess from a diff perspective), so recommend reviewing just the second commit which contains the new implementation, which is much better from a diff perspective, and aligns with the approach we will probably end up with upstream in otel.