Repository navigation
test(otel): cover replay and chained execution - #796
zhongkechen wants to merge 13 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Preserve the shared workflow main reference and active OTel coverage for cases 21-24 and 27. Keep cases 25-26 deferred and retain the merged case 11 replay fix, dependency updates, and MicroVM worker changes. Validation: 81 focused conformance and workflow tests pass. Formatting, lint, whitespace, and the shared workflow contract checks pass.
Defer cloud case 21 in both views until its shared history requirement accepts both wait-completion orderings. Preserve local replay coverage. Exercise case 23 helper failures and unexpected successes through the public runner in both views. Each error guard is protected by tests that fail when it is removed. Validation: 85 focused conformance and workflow tests pass. Package formatting, lint, and whitespace checks pass.
This comment has been minimized.
This comment has been minimized.
Enable cases 25 and 26 in both cloud views and the local test suite. Keep their regressions failing until the prerequisite fixes merge. Preserve the callback completion export-time assertion and both later replay barriers. Use raw span observations instead of configurable proof-file reporting, and document the intended empty-history contract. Validation: 89 focused tests pass and four expected regression cases fail. Formatting, lint, and the 63 template contract tests pass.
| assert len(targets) == 1, ( | ||
| "target completion must reach the exporter before the observed step" | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if not trace.get_current_span().get_span_context().is_valid: | ||
| raise RuntimeError(f"No active span for conformance.{label}") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
| assert len(targets) == 1, ( | ||
| "target completion must reach the exporter before the observed step" | ||
| ) |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_ysa6hd7vcryj2bv5pbshd64jxt
[P1] Land the prerequisites before merging this coverage
These tests are collected by the root testpaths and run through ordinary CI's hatch run test:cov. The base's complete_callback_success() still does not populate UpdatedOperationIds, preventing the terminal export required here. This diff contains none of the prerequisite fixes, and the PR's validation reports four failures across cases 25 and 26. Merging therefore breaks ordinary CI, beyond the cloud conformance jobs. Land the required runtime/testing fixes and rebase before merging these tests, preserving their regression assertions.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| if not trace.get_current_span().get_span_context().is_valid: | ||
| raise RuntimeError(f"No active span for conformance.{label}") |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_or7fkvw6kbgygouxnc37cdzunw
[P3] Revisit the untested probe rejection guards
Rule 4, “Every branch has a test that fails without it,” remains unmet here and in case 23's identical probe guard. Every scenario supplies an active SDK span, including the no-ambient cases; case 23's negative tests change the helper outcome, not the tracing context. Removing either rejection leaves the existing assertions unchanged. Return to the design phase under rule 8 to establish whether these guards add a necessary contract beyond the existing parentage assertions and choose the minimum demonstrably tested approach.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
Codex AI reviewTwo unresolved findings:
Reviewed all changed files and rules 1–9. Tests were not executed during this read-only review. Reviewed commit |
yaythomas
left a comment
There was a problem hiding this comment.
per description, ready to merge once Python SDK PR #865: correct terminal telemetry status and timing and Python SDK PR #868: record callback terminal operation updates land. The inline comments are suggestions.
| contents: read | ||
| id-token: write | ||
| uses: aws/aws-durable-execution-conformance-tests/.github/workflows/opentelemetry-orchestrator.yml@d4b7df927ea7eb7022bcca339d2cddd387d3d650 | ||
| uses: aws/aws-durable-execution-conformance-tests/.github/workflows/opentelemetry-orchestrator.yml@main |
There was a problem hiding this comment.
With @main, every later push to the conformance repository's main branch changes the code that runs in this job. This job has id-token: write and actions: write, and it passes seven secrets to that code. A pinned commit keeps each such change behind a reviewed pull request in this repository. Can this pin the conformance repository's current main commit instead of @main?
uses: aws/aws-durable-execution-conformance-tests/.github/workflows/opentelemetry-orchestrator.yml@0eaf7341c0c7ff937e6a4bf6bda1682ff07e7502Three things I checked support this:
- Dependabot already keeps this pin current. It last bumped it in Python SDK PR #844: bump the actions-deps group with 6 updates. So the pin costs a reviewed Dependabot update, not manual work.
- The commit above has what this PR needs from the shared workflow: the
case_countinput, which defaults to 27, and the removal of the long-running jobs. The current pin still hard-codescase_count: 20. telemetry-conformance-tests.ymlpins the same conformance commit as the current pin here. With@mainhere, the two callers of the same shared repository would follow different rules.
The workflow test would then check for a full commit SHA, and lines 285 and 286 of the package README would change with it:
shared_ref = re.search(re.escape(orchestrator) + r"(\S+)", workflow)
assert shared_ref is not None
assert re.fullmatch(r"[0-9a-f]{40}", shared_ref.group(1))One related cleanup, which is optional. At the commit above, the orchestrator runs neither suite for a manual run unless phase is short, and it ignores delay_seconds. So a manual run with launch or check runs no tests, and the header comment on line 7 still mentions the long-running cycle. Can the phase and delay_seconds inputs (lines 26 to 39) and the lines that pass them (87 and 88) go in this PR too?
| def counted(step_context: Any) -> str: | ||
| targets = terminal_target_exports() | ||
| assert len(targets) == 1, ( | ||
| "target completion must reach the exporter before the observed step" | ||
| ) | ||
| calls.append(value) | ||
| return step(step_context) |
There was a problem hiding this comment.
An AssertionError inside a step body is a step failure like any other. This step has no retry strategy, so the SDK retries it with RetryPresets.default() (step.py, line 334). In the current failure, the first attempt fails here and a later attempt passes. Pytest then reports the shifted event IDs, assert [(2, 3), (11, 14), (18, 21)] == [(2, 3), (8, 11), (15, 18)], and the message on line 113 appears only in the captured log. Can the step body record what it saw, and the test assert on that after the run, so the failure names the late export?
calls: list[tuple[str, int]] = []
...
def counted(step_context: Any) -> str:
calls.append((value, len(terminal_target_exports())))
return step(step_context)
...
# Before the gates check, so a late export is reported as such.
assert calls == [("target", 1)], (
"target completion must reach the exporter before the observed step"
)
assert gates == [(2, 3), (8, 11), (15, 18)]I tried this on this branch merged onto current main. Both views then fail with this message and assert [('target', 0)] == [('target', 1)], and the failing run takes 1.3 seconds instead of about 10, because no step retry runs.
synchronized_probe in test_user_function_context.py asserts inside step bodies too (line 110). A failure in a step body there would also go through step retries before pytest sees it.
| # Intended local history contract from #862. The exact empty Payload | ||
| # service shape remains unverified against deployed AWS Lambda. |
There was a problem hiding this comment.
The cloud run for this revision already shows the service's shape for the detailed history. In all four jobs of the OTel conformance run for this revision, the saved case 25 history has "Error": {"Payload": {}, "Truncated": false} on the errorless CallbackFailed event. The only failure reported for case 25 in those jobs is the span status check. So the expectation on lines 276 to 279 matches the service. Can this comment, and the README paragraph on lines 144 to 149, cite that run instead of calling the shape unverified?
Two facts explain the result:
- The shared runner sends this failure without an
Errorargument. Erroris the HTTP payload member ofSendDurableExecutionCallbackFailure. When it is omitted, botocore sends{}as the request body (RestJSONSerializer._serialize_empty_body). So for a boto3 caller, an omitted error andError={}are the same request.
The cloud requirement checks only the detailed history. So the metadata-only shape on lines 280 to 282 is still the part without cloud evidence.
| - Case 25 needs terminal failure status without diagnostic details | ||
| ([#853](https://github.com/aws/aws-durable-execution-sdk-python/issues/853)) | ||
| and consistent empty callback error history | ||
| ([#862](https://github.com/aws/aws-durable-execution-sdk-python/issues/862)). | ||
| - Case 26 needs completion discovery and export | ||
| ([#854](https://github.com/aws/aws-durable-execution-sdk-python/issues/854)), | ||
| callback update production | ||
| ([#860](https://github.com/aws/aws-durable-execution-sdk-python/issues/860)), | ||
| and invocation-input acknowledgment | ||
| ([#861](https://github.com/aws/aws-durable-execution-sdk-python/issues/861)). | ||
| - The additional case 17 callback-error regression also needs snapshot isolation | ||
| ([#852](https://github.com/aws/aws-durable-execution-sdk-python/issues/852)). | ||
| The existing case 17 handler and supplied-error coverage remain active. |
There was a problem hiding this comment.
This list no longer matches what the tests need. Two of the listed issues are closed, and the tests pass without the fixes for two others. Can it say that case 25 waits on Python SDK #853: report terminal failures and keep exception events within span bounds and case 26 waits on Python SDK #860: record callback terminal completions as updated operations, and drop the other entries?
Two things I checked support this:
- Python SDK #852: isolate terminal plugin error snapshots closed with Python SDK PR #866: isolate terminal plugin error snapshots on 2026-10-28. Python SDK #862: preserve empty callback error history closed with Python SDK PR #870: preserve empty errors in state and history on 2026-10-30.
- I merged each open fix onto this branch with current
mainand ran both test files. Both case 25 tests pass with Python SDK PR #865: correct terminal telemetry status and timing alone. Both case 26 tests pass with Python SDK PR #868: record callback terminal operation updates alone. Python SDK PR #867: acknowledge delivered invocation input alone does not fix case 26, and case 26 passes without Python SDK PR #869: export visible replay completions.
The description lists the same prerequisites, so it may need the same change.
Add OpenTelemetry conformance handlers for user-function and callback tracing contexts (22–23), invocation retries reported as
RETRYING/UNSET(24), caught callback failure replay (25), external callback completion and later replay (26), and a chained child that suspends and resumes through a durable wait (27). These handlers exercise the public Core 3 / OTel 2 APIs in both telemetry views.Case 21 retains its handler and local replay regression tests, but cloud coverage is explicitly
NOT_IMPLEMENTEDuntil the shared history requirement accepts both valid wait-completion orderings. Cases 25–26 are enabled in both cloud suites and the ordinary local test suite. Keep their failures as merge gates until the prerequisite bug fixes merge; do not skip, xfail, or suppress them. The existing case 17 handler remains active; its additional callback-error regression is deferred.The case 11 callback/replay fix and regression test have merged in #864 and are absent from this PR's diff against
main.Review decisions and simpler alternatives
InvocationCompletedand event 6 toWaitSucceeded. The execution-view cloud run observedWaitSucceeded63 ms earlier while the execution succeeded withbefore-after. The wait timer and invocation cleanup can finish in either order. Follow the review's deferral option: remove both cloud functions and their build targets and declare both cases pending. The invocation catalog imposes the same ordering, so deferring only the execution view leaves the same risk. Increasing the wait merely reduces the race; it does not establish an ordering guarantee. Re-enable cloud case 21 after a coordinated shared-requirement correction preserves the replay assertions while accepting both orders. Local tests still require each step body to run once,PENDING/SUCCEEDEDinvocations, and one export per completed operation in the execution view.Workflow and prerequisite fixes
The shared OpenTelemetry workflow and test catalog default to the conformance repository's
mainbranch. Manual runs retain theconformance_test_refoverride; SDK handlers run from the commit under test. The orchestrator runs invocation and execution suites. Legacy long-running templates and local contract tests remain, but the orchestrator no longer schedules that suite.Cases 25 and 26 are registered in the SAM template and Makefile, and their local tests are collected by the existing root test configuration and
hatch run test:cov. The prerequisite issues remain open:Case 25 asserts the intended local-runner contract in #862: detailed callback-failure history includes
Payload: {}withTruncated: false; metadata-only history omits the payload and setsTruncated: true. This exact emptyError.Payloadservice shape remains unverified against deployed AWS Lambda. The restored test inspects raw spans and records the invocation when each span is exported; timestamp-only checks cannot establish delivery timing. It uses ordinary pytest diagnostics, with no configurable proof-file output or JSON reporting mechanism.Case 26 asserts inside the observed step that completion has already reached the exporter, and preserves raw export-count checks at both later barriers and final completion.
The additional case 17 regression remains deferred. Its source is preserved in 6484e577; before restoring it, correct its history expectations so included diagnostic payloads are untruncated and metadata-only history neither exposes payloads nor reconstructs error details.
Validation
On Python 3.13.17:
hatch run test:all packages/aws-durable-execution-sdk-python-conformance-tests-otel/tests .github/scripts/tests/test_opentelemetry_conformance_workflow.py -q --tb=short: 4 failed, 89 passed. The failures are intentional regression gates against the current base: case 25 in both views is missing the empty detailed-history payload ([Testing]: Preserve empty callback error history without changing runtime errors #862); case 26 in both views observes zero terminal exports inside the observed step, causing an extra retry and shifted history gates ([OTel]: Export unvisited replay completions with the correct parent context #854/[Testing]: Record callback terminal completions as updated operations #860/[Testing]: Acknowledge invocation input without losing pending updates #861). Later assertions remain enabled, including case 25's terminal ERROR status checks for [OTel]: Report terminal failures and keep exception events within span bounds #853.git diff --check: passed.Existing test expectation changes
test_templates_map_every_otel_requirement,test_templates_declare_only_the_expected_functions,test_function_names_identify_the_requirement,test_makefile_builds_every_function,test_every_registered_handler_exists, andtest_chained_invoke_functions_point_at_their_target: include cloud cases 22–27 and case 27's targets; case 21 stays out of the cloud mappings/build targets while its shared history contract is pending.test_active_callback_and_retry_handlers_keep_their_mapping: verifies that cases 25 and 26 use their restored replay handlers, alongside the existing case 17 and 24 mappings.test_handler_modules_are_exactly_the_expected_set: includes the new active handlers plus case 21's retained local handler.test_deferred_requirements_are_explicit: only case 21 remains pending in both views, with a reason and complete accounting for all 27 cases. Cases 25 and 26 are active requirements.test_callback_probes_keep_their_actual_sdk_parent: adds unexpected helper failure/success outcomes to protect both error checks; the original successful-scenario assertions are unchanged.test_opentelemetry_conformance_caller_uses_current_workflow_contract: expects the shared workflow and catalog to default tomain.Case 27 companion PRs: JS #1068 and Java #824.