Record processed logs before export complete and reject on shutdown - #8698
Record processed logs before export complete and reject on shutdown#8698anuraaga wants to merge 3 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-08-07 03:06 UTC Respond to 2 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
| CompletableResultCode result; | ||
|
|
||
| synchronized (exporterLock) { | ||
| logProcessorInstrumentation.finishLogs(1); |
There was a problem hiding this comment.
I put it before the export call to match what languages with synchronous export like Go and Python must be doing
There was a problem hiding this comment.
Calling it before does also mitigate against the risk of export throwing synchronously.
Probably worth a comment since the placement seems unintuitive at first glance.
| "simple_log_processor"))))); | ||
|
|
||
| // Logs rejected after to call to shutdown, regardless of completion so no join. | ||
| processor.shutdown(); |
There was a problem hiding this comment.
Interestingly, provider.shutdown() causes loggers to reject immediately it seems, I guess the processor's handling will be for that small race within emitLog itself (emitLog started, logger and processor disabled from other thread, emitLog finishes)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8698 +/- ##
============================================
+ Coverage 91.62% 91.66% +0.03%
- Complexity 10328 10349 +21
============================================
Files 1003 1003
Lines 27140 27205 +65
Branches 3188 3198 +10
============================================
+ Hits 24868 24937 +69
+ Misses 1566 1564 -2
+ Partials 706 704 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| private final AtomicInteger logsNeeded = new AtomicInteger(Integer.MAX_VALUE); | ||
| private final BlockingQueue<Boolean> signal; | ||
| private final AtomicReference<CompletableResultCode> flushRequested = new AtomicReference<>(); | ||
| private final AtomicBoolean isShutdown = new AtomicBoolean(false); |
There was a problem hiding this comment.
The BatchLogRecordProcessor also has an isShutdown field. Since we need it down in the Worker for instrumentation, can we get rid of it in the parent and just delegate BatchLogRecordProcessor#shutdown to Worker#shutdown()?
| CompletableResultCode result; | ||
|
|
||
| synchronized (exporterLock) { | ||
| logProcessorInstrumentation.finishLogs(1); |
There was a problem hiding this comment.
Calling it before does also mitigate against the risk of export throwing synchronously.
Probably worth a comment since the placement seems unintuitive at first glance.
#8680
I will apply the same to spans after this one to confirm the pattern
On top of changing the order of recording, this implements handling of shutdown processor to emit
already_shutdownand reject the log (a non-metric behavior change). For what it's worth, the behavior would have been less confusing if thaterror.typewas there at the time 😂