Skip to content

Skip scheduling a redundant timeout task for synchronously-completed metric exports - #2

Draft
jaipilot[bot] wants to merge 1 commit into
mirror/upstream-pr-8684from
jaipilot/pr-1-22AcgEM7hJXK
Draft

Skip scheduling a redundant timeout task for synchronously-completed metric exports#2
jaipilot[bot] wants to merge 1 commit into
mirror/upstream-pr-8684from
jaipilot/pr-1-22AcgEM7hJXK

Conversation

@jaipilot

@jaipilot jaipilot Bot commented Aug 22, 2026

Copy link
Copy Markdown

Behavior lock

Ran the existing PeriodicMetricReaderTest suite (25 tests covering the new setExporterTimeout builder API, default 30s timeout, explicit timeout with/without batching, and slow-exporter timeout enforcement) against the unmodified PR head — all passed. This was recorded as the behavior baseline; the same command was re-run against the final candidate and still passes, proving no observable behavior changed.

Change

PeriodicMetricReader.Scheduled.applyTimeout() unconditionally scheduled a timer task via scheduler.schedule(...) and registered a whenComplete callback to cancel it. Because CompletableResultCode.whenComplete() runs its callback synchronously when the target result is already done, every synchronously-completed export (in-memory/logging exporters, or exporters that already return a cached ofSuccess()/ofFailure()) paid the cost of scheduling a timer and immediately cancelling it, once per export call (and once per batch when batching is enabled).

Added a fast path: when the export result is already done, return CompletableResultCode.ofSuccess()/ofFailure() directly — the exact same isSuccess()/isDone()/getFailureThrowable() (always null) that the prior scheduled-then-cancelled path produced. The asynchronous/slow-exporter timeout path is untouched.

Proof

Added applyTimeout_skipsSchedulingForSynchronouslyCompletedExport, a deterministic operation-count test using the repository's existing mocked-ScheduledExecutorService pattern (see startOnlyOnce). It asserts scheduler.schedule(...) is never invoked when the exporter completes synchronously. Verified this test fails against the original PR-head implementation (Mockito NeverWantedButInvoked, 1 actual invocation) and passes against the candidate (0 invocations) — a real, evidence-backed elimination of wasted work, not a timing guess.

Verification

  • ./gradlew :sdk:metrics:test --tests "io.opentelemetry.sdk.metrics.export.PeriodicMetricReaderTest" — passed before and after.
  • ./gradlew :sdk:metrics:check — passed (tests, spotless, checkstyle, jApiCmp). No public API diff.

Scope notes

Cleanup and modernization passes were evaluated and found no additional safe, in-scope improvement: no dead code, no build/version files were touched by the original PR, and the changed code requires no dependency upgrade. This mirror does not claim the original upstream work and is not intended to be merged or opened against the upstream repository.


Generated by JAIPilot Cloud for #1 from Anthropic session sesn_01VgrUCPurgr22AcgEM7hJXK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants