feat(sdk-metrics): align PeriodicMetricReader export timeout semantics - #8684
Rajkaran-122 wants to merge 16 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-09-14 18:32 UTC Resolve merge conflicts. Respond to 2 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
…cheduler Replace scheduler-based withTimeout() with CompletableResultCode.join(), matching the established pattern in BatchSpanProcessor and BatchLogRecordProcessor. The previous approach used scheduler.schedule() which throws RejectedExecutionException during shutdown because the scheduler is intentionally shut down before the final export flush.
|
Hi @Rajkaran-122 — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
…ication - Added setExporterTimeout() API to PeriodicMetricReaderBuilder with 30-second default per spec - Removed @SInCE 1.40.0 annotations as requested by reviewer - Updated API-diff to reflect new public API - Added comprehensive tests for timeout behavior - Implemented conditional timeout enforcement (only when explicitly configured) - Addressed blocking concern by making timeout opt-in via setExporterTimeout()
…Reader Add asynchronous timeout enforcement for MetricExporter operations in PeriodicMetricReader using a dedicated timeout executor. This preserves the existing asynchronous scheduling and batching behavior while enforcing the spec-required 30-second default export timeout. Changes: - Add dedicated ScheduledExecutorService for timeout scheduling - Implement applyTimeout() method with asynchronous timeout enforcement - Preserve async batch processing with Iterator-based sequential execution - Fix Error Prone warnings (UnusedVariable, PreferJavaTimeOverload) - Add timeout enforcement test The timeout executor is shut down after final export completes to avoid RejectedExecutionException during shutdown. Timeout enforcement fails the result when timeout expires without blocking the periodic scheduler. Resolves CI compilation failures in PR open-telemetry#8684.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8684 +/- ##
============================================
+ Coverage 91.23% 91.26% +0.02%
- Complexity 10639 10668 +29
============================================
Files 1010 1010
Lines 28696 28717 +21
Branches 3682 3687 +5
============================================
+ Hits 26182 26209 +27
+ Misses 1710 1702 -8
- Partials 804 806 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… jack-berg review - Remove separate timeout executor and reuse existing scheduler - Increase scheduler from 1 to 2 threads to handle both periodic exports and timeout tasks - Simplify applyTimeout() by removing AtomicBoolean/timedOut state tracking - Combine SlowMetricExporter and FastMetricExporter into single DelayingMetricExporter - Add RejectedExecutionException handling for shutdown race condition - Fix BooleanParameter warnings in tests
|
Thanks, @jack-berg sir. I've updated the implementation based on your feedback:
I also handled the scheduler-shutdown case in applyTimeout() because the final shutdown export can race with scheduler shutdown when scheduling the timeout task. All relevant sdk:metrics formatting, compilation, and PeriodicMetricReaderTest checks pass. |
|
@jack-berg sir , please review this pr. |
|
I ran JAIPilot Cloud against this exact PR head. It found one deterministic follow-up: when an export is already complete, skip creating and immediately cancelling the timeout task. The focused path changed from 1 schedule/cancel pair to 0. Baseline passed 31 focused tests, candidate passed 32 including the new regression test, and both clean Cloud-generated draft and evidence: skrcode#2 Feel free to merge or cherry-pick if it fits the intended timeout semantics. |
|
@jack-berg sir, please review the pr. |
- Change default timeout from min(interval, 30s) to exactly the configured interval - Remove DEFAULT_EXPORT_TIMEOUT_MILLIS constant (no longer used) - Update Javadoc for both timeout overloads to document new default behavior - Add Javadoc explaining timeout applies to each batch when maxExportBatchSize is configured - Update test to verify default timeout equals interval - Add test to verify explicit timeout is preserved with large intervals
|
@jack-berg sir, please review the pr. |
|
I'm away I'll get back to this when I can |
|
@jack-berg sir PTAL. |
This is from the PR description but is no longer accurate. Can you review the whole PR description and make sure its aligned with the current state of the PR? Thanks. |
…test improvements Critical Concurrency Fix: - Fixed critical backpressure issue where timeout firing released exportAvailable while underlying exporter.export(...) was still running - Separated backpressure (tied to raw export result) from timeout reporting (tied to timeout result) in doRun() - Removed applyTimeout() from exportMetrics() to apply timeout only at the top level for reporting, not for backpressure - This prevents concurrent exports from accumulating when exporters are slow Test Improvements: - Replaced per-export Thread spawning with shared ExecutorService in tests - Added deterministic timeout regression test using LogCapturer to verify timeout warning was actually logged - Added concurrency semantics test to prove timeout does not release backpressure - Fixed Error Prone warning about ignored Future return value All tests pass, spotless clean, git diff --check clean.
Please don't ignore comments. Also, respond to comments and mark resolved if you feel your changes resolve them. |
Ok sir . |
|
@jack-berg sir PTAL . |
|
@Rajkaran-122 thank you for the persistence on this one across many rounds of feedback. I still have issues with the implementation - in particular that the timeout is enforced at the batch level instead of individual export level as the spec said. I tried to take this PR branch and adapt it to meet all the requirements, but ran into spiraling complexity and code that was really hard to grok. Concurrency + high complexity is a recipe for bugs. After stepping back, I think the difficulty we've had is a signal that we're fighting the existing architecture. Adding a per-batch exporter timeout on top of I apologize for not catching this earlier. Some of the review comments I left pushed the design further in that direction instead of questioning whether the shape was right, and by the time it became clear, we'd already invested several iterations. I'd like to change direction: refactor |
- Replace scheduler-thread exports with dedicated worker thread architecture - Scheduler now only emits periodic tick signals; worker owns export execution - Use blocking signal queue for serial processing of ticks, flushes, and shutdown - Implement per-batch timeout enforcement that waits for actual completion - Add regression test proving no concurrent exporter invocations after timeout - Prevents concurrent Export calls as required by metrics specification The timeout behavior now correctly: - Applies timeout to each individual export(batch) invocation - Reports timeout after configured duration elapses - Waits for actual export completion before proceeding to next batch - Prevents concurrent exporter operations when timeout expires This addresses issue open-telemetry#8311 and aligns with PR open-telemetry#8827's dedicated-worker design.
|
@jack-berg sir Thanks for the detailed feedback and for pointing out the architectural issue with the previous approach. I reworked the implementation around the dedicated-worker model. The scheduler now only signals work, while a dedicated worker handles collection and export sequentially. The exporter timeout is applied independently to each batch. Since the underlying exporter operation cannot be cancelled, the worker waits for its actual completion before starting the next batch, preventing concurrent exporter invocations after a timeout. I also added a deterministic regression test covering this timeout/backpressure behavior. The |
|
|
@jack-berg sir PTAL . |
|
@jack-berg sir Would it be okay if I add the |
3 participants
Fixes #8311
Summary
Align
PeriodicMetricReaderexport timeout behavior with the Metrics specification by enforcing an exporter timeout for each export batch.Changes
PeriodicMetricReaderBuilderPeriodicMetricReaderTesting
git diff --checkNotes
This change updates export timeout behavior without affecting metric collection or normal scheduling semantics.