Fix and re-enable the flaky celery docker tests - #4964
Draft
Pissinatti-py wants to merge 3 commits into
Draft
Conversation
The run span is ended from the task_postrun signal, which celery fires after backend.mark_as_done() has already stored the task result. result.get() can therefore return before the worker has ended and exported the run span, so reading the exporter straight afterwards races the worker. Add wait_for_spans(), which polls until the expected number of spans has arrived and raises an AssertionError on timeout, so a message that never arrives fails the test instead of hanging it. Apply it to test_use_span_links, which is not skipped but shares the same race. Caveat on the flakiness: this race could not be reproduced locally. A tight apply_async/result.get() loop saw a short read in 0 of 150 iterations, and the suite passed 25/25 runs. What is confirmed is the ordering in celery/app/trace.py, where backend.mark_as_done() runs on the success path and send_postrun() only in the enclosing finally block; that window is also the diagnosis recorded in c4639ee. The wait is therefore justified by the code path rather than by an observed failure, and it is a no-op when the worker wins the race, as it does on every machine tested here. Refs open-telemetry#653
Un-skip four of the tests disabled with "inconsistent test results" and correct assertions that no longer describe what the instrumentation does: - test_fn_task_apply_async and test_fn_task_delay asserted that the run span is in a different trace from the publish span. before_task_publish injects the trace context into the message headers and task_prerun extracts it again, so with the default use_span_links=False the run span is a child in the same trace. test_fn_task_apply_async also expected a run span name without its "run/" prefix. - test_instrumentation_info asserted scope names of "apply_async/<module>" and "run/<module>". Both spans come from a single tracer whose scope is opentelemetry.instrumentation.celery; the prefixes belong to span names, not scopes. Renamed to test_instrumentation_scope and rewritten against instrumentation_scope, as instrumentation_info is deprecated. Select spans by their celery.action attribute rather than unpacking the exported list positionally: export order is span end order, which is not guaranteed once more than one task is in flight. Caveat on the flakiness: these four did not fail intermittently, they failed deterministically, 25 runs out of 25, for the stale-assertion reasons above. The produce/consume race described in open-telemetry#653 is real, but it is not what kept these tests red, and re-enabling them would have failed without the assertion fixes. Refs open-telemetry#653
test_apply_async_previous_style_tasks subclassed celery.task.Task and overrode apply_async as a classmethod. celery.task was removed in Celery 5, so the test raised AttributeError against the pinned celery==5.3.6 no matter the timing. The classmethod form cannot be kept either: super().apply_async accessed from a classmethod is unbound and would receive no self. Port it to an instance method override on app.Task, which is the equivalent scenario on Celery 5 and preserves the original intent -- apply_async must still be traced when a Task subclass overrides it and delegates to super(). Register the task on the app so the worker can execute the nested publish, and identify the two run spans by message id rather than by position, since the eagerly applied outer task and the published inner task differ only in their ids. Caveat on the flakiness: this test was skipped as "inconsistent test results" alongside the genuinely racy ones, but it was not flaky. It was broken unconditionally by a Celery API removal, which the shared "inconsistent test results" skip reason hid for as long as the skip stayed in place. Fixes open-telemetry#653
|
Pull request dashboard statusWaiting on the author · refreshed 2026-08-20 17:54 UTC Move out of draft to request review. Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Re-enables the five celery docker tests skipped with
inconsistent test resultssince 2021.Two separate problems were hidden behind that one skip reason:
The race described in the issue. The run span is ended from the
task_postrunsignal, which celery fires afterbackend.mark_as_done()has already stored the result (celery/app/trace.py—mark_as_doneon the success path,send_postrunonly in the enclosingfinally).result.get()can therefore return before the worker has exported the run span. Addedwait_for_spans(), which polls until the expected count arrives and raises anAssertionErroron timeout so a message that never arrives fails the test rather than hanging it. Applied totest_use_span_linkstoo, which was not skipped but shares the race.Stale assertions. Un-skipping alone did not make these pass:
test_fn_task_apply_async/test_fn_task_delayasserted the run span is in a different trace from the publish span.before_task_publishinjects trace context into the message headers andtask_prerunextracts it, so with the defaultuse_span_links=Falseit is a child in the same trace.test_fn_task_apply_asyncalso expected a run span name without itsrun/prefix.test_instrumentation_infoasserted scope names ofapply_async/<module>andrun/<module>; both spans come from one tracer scopedopentelemetry.instrumentation.celery. Renamed totest_instrumentation_scopeand rewritten againstinstrumentation_scope.test_apply_async_previous_style_taskssubclassedcelery.task.Task, removed in Celery 5, so it was broken unconditionally rather than flaky. Ported to an instance-methodapply_asyncoverride onapp.Task(the classmethod form cannot work —super().apply_asyncfrom a classmethod receives noself).Spans are now selected by their
celery.actionattribute instead of unpacked positionally, since export order is span end order.Gap worth flagging: I could not reproduce the timing race locally — 0 short reads in 150 iterations of a tight
apply_async/result.get()loop, and 25/25 clean suite runs. The ordering above is confirmed in celery's source and matches the diagnosis recorded in c4639ee, so the wait is justified by the code path; but what actually kept these five red was staleness, not timing.Fixes #653
Type of change
How Has This Been Tested?
pytest celery/fromtests/opentelemetry-docker-tests/tests: 18 passed, 0 skipped (previously 13 passed, 5 skipped)ruff checkandruff format --checkcleanTest configuration: Python 3.12,
celery==5.3.6/kombu==5.3.5as pinned intest-requirements.txt, redis broker/backend from the repo'sdocker-compose.yml. This was a celery-only virtualenv rather than the fulltox -e docker-testsenvironment, which would not build on my machine (missingdefault-libmysqlclient-dev/unixodbc-dev) — so CI is the first run against the real env on Python 3.11. Opened as a draft for that reason.Does This PR Require a Core Repo Change?
Checklist:
Disclosure: this change was AI-assisted (Claude Opus 5).