Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-11-03 17:31 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
chrikrah
left a comment
There was a problem hiding this comment.
@Dotify71 blocking: the title says force_flush respects timeout_millis, but the loop checks the deadline only after it holds _export_lock. While the worker drains a slow exporter, force_flush(100) waits 2.95s past its deadline.
A timed lock wait fixes that, with a guard: deadline defaults to math.inf, and a lapsed one goes negative. One slow export still overruns until export takes a timeout. That is the ask in #4568, and Refs #4568 fits better than Fixes.
# git archive 2c489c0, Python 3.12, opentelemetry.sdk from the exported tree
$ python -m pytest opentelemetry-sdk/tests/shared_internal/test_batch_processor.py -q -p no:cacheprovider
27 passed, 2 warnings in 7.46s
# _shared_internal/__init__.py from base 7fd5f6b, tests kept
2 failed, 25 passed (test_force_flush_returns_false_when_timeout_exceeded, both processors)
# 0.5s exporter, max_export_batch_size=1, 6 spans ended, then force_flush(timeout_millis=100)
$ python -I lockwait.py
force_flush(100) -> True after 2.95s # head, queue already drained by the worker
force_flush(100) -> False after 0.10s # lock acquired with the remaining time, finite deadline
# that variant fails the new timeout test: its time.time side_effect holds two values
# Python 3.12.3, threading.Lock().acquire(timeout=...)
timeout=math.inf -> OverflowError timestamp out of range for platform time_t
timeout=-0.01 -> ValueError timeout value must be positive
# not run: tox lint, the OTLP exporters
Want the probe as a test here?
Yes, please adding the probe as a test here would be fantastic. Please feel free to share the test probe or push/suggest it, and I'll include it in the branch :)) |
- Acquire _export_lock with timeout=remaining when deadline is finite - Guard against negative timeout values and math.inf OverflowError - Add test_force_flush_returns_false_when_lock_acquisition_times_out
chrikrah
left a comment
There was a problem hiding this comment.
@Dotify71 the timed lock wait fixes my blocker at 58c2682, and your lock-holder test is the probe I offered, so mine isn't needed. From my perspective this can be merged.
# git archive 58c2682, Python 3.12.3, opentelemetry.sdk imported from the export
$ python -m pytest opentelemetry-sdk/tests/shared_internal/test_batch_processor.py -q -p no:cacheprovider
29 passed, 2 warnings
# _shared_internal/__init__.py from 2c489c0, my last review's head, tests kept
2 failed, 27 passed # test_force_flush_returns_false_when_lock_acquisition_times_out, both processors
$ python -I lockwait.py # 0.5s exporter, max_export_batch_size=1, 6 spans, then force_flush
force_flush(100) -> False after 0.10s # 58c2682 (2c489c0: True after 2.95s)
force_flush() -> True after 2.85s # no timeout, math.inf path
_export(EXPORT_ALL, deadline=now - 1) -> False
# not run: tox lint, the OTLP exporters
An export already in flight still runs past the deadline, which Refs #4568 covers, and the math.inf default is what herin049 suggested on #4982. Could the description list the lock-acquisition test too? It still says 27 passed.
Thanks for the thorough review and testing, @chrikrah I've updated the description to list |
Description
Refs #4568
Part of #5684
In
BatchProcessor(opentelemetry-sdk/src/opentelemetry/sdk/_shared_internal/__init__.py),force_flush(timeout_millis)previously accepted atimeout_millisargument but never used it, blocking indefinitely until all telemetry was exported regardless of the specified deadline:Changes Made
_export:deadline: float = math.infto_export()._export_lockwithtimeout=remainingwhen deadline is finite (remaining = deadline - time.time()), returningFalseimmediately if the deadline has already passed (remaining <= 0) or if lock acquisition times out.OverflowErroronmath.infand negative timeout values forthreading.Lock.acquire().if time.time() >= deadline:at each batch iteration, returningFalseif the deadline is exceeded before all batches are exported, andTrueif all batches were successfully drained.force_flush:deadline = time.time() + (timeout_millis / 1000) if timeout_millis is not None else math.inf.self._export(BatchExportStrategy.EXPORT_ALL, deadline).test_batch_processor.pycovering:test_force_flush_returns_true_when_all_exportedtest_force_flush_returns_false_when_timeout_exceeded(tested with patchedtime.time)test_force_flush_returns_false_when_lock_acquisition_times_outtest_force_flush_returns_false_when_shutdown.changelog/5842.fixed.Type of change
How Has This Been Tested?
pytest opentelemetry-sdk/tests/shared_internal/test_batch_processor.py -v(all 29 tests passed).pytest opentelemetry-sdk/tests/trace/export/test_export.py opentelemetry-sdk/tests/logs/test_export.py -v(all 44 tests passed).ruff format --check opentelemetry-sdkandruff check opentelemetry-sdk.Checklist: