Skip to content

refactor: share operation span status mapping - #896

Open
zhongkechen wants to merge 2 commits into
mainfrom
refactor-otel-status-review-865-20261101
Open

zhongkechen wants to merge 2 commits into
mainfrom
refactor-otel-status-review-865-20261101

Conversation

@zhongkechen

Copy link
Copy Markdown
Contributor

Issue #, if available

Follow-up to #865, merged as 1653f08. Addresses its three remaining review comments:

Description of changes

Execution and Invocation views duplicated the operation status rules, so each correction required editing both. Both now call set_operation_span_status in a shared module. Error details still take precedence, FAILED/TIMED_OUT/STOPPED without details remain ERROR, SUCCEEDED remains OK, and other errorless statuses remain UNSET.

The README states the timestamp rules directly, including child-end extension, the 1 µs adjustments, and clock fallback. It removes the misleading statement that checkpointless contexts generally start at completion.

test_operation_status_and_exception now looks up literal expected statuses instead of recomputing the production rule. Expected values are unchanged, and iteration over OperationStatus ensures a new enum member requires an explicit expectation. test_failed_operation_without_type_or_message_uses_fallback also asserts ERROR with an empty status description, preserving the existing no-message behavior.

Design

  • Design note (rule 3): not needed; this is an internal refactor and documentation clarification with no public API change.
  • Simplification (rule 2): replace both copies of the three-branch mapping with one function. Exception recording uses the existing recorder's no-error return. Timestamp selection remains in each view.
  • Root cause: duplicated mapping logic, a test expectation that repeated that logic, and documentation that described a fallback as general checkpointless-context behavior.
  • Net source change: +6 lines across three modules, with one shared mapping replacing two copies. No public API members added.

Validation

  • Before the extraction, the literal-table status suite passed: 164 tests.
  • hatch run dev-otel:test -qq --tb=short -o faulthandler_timeout=60: 874 passed, including public-handler replay tests.
  • hatch run dev-otel:typecheck: no issues in 46 files.
  • OTel package hatch fmt --check: passed.
  • Core and OTel hatch build: passed.
  • hatch run test-wheel-otel:test -qq --tb=short: 874 OTel tests and 12 lifecycle compatibility tests passed; installed source bytes and wheel hashes were verified.
  • Independent mutations of error precedence, failed-status handling, success handling, preservation of UNSET, and missing-message fallback caused 34, 12, 4, 16, and 2 test failures respectively. All mutations were restored; the final 164 status tests passed and the restored module matches the tested wheel.
  • Commit-message lint and git diff --check: passed.
  • Python 3.13.5 and Hatch 1.17.0. Full suites ran outside the sandbox after the restricted source run stalled in its first local-runner test and was interrupted.

Assumptions

  • Status descriptions and exception events are checked against a real OTel TracerProvider and InMemorySpanExporter in both views. Existing public-handler tests exercise replay and caught failures.
  • Deployed Lambda/exporter conformance was not rerun locally for this refactor. Runtime timing and lifecycle contracts are unchanged.

Decisions not covered by the issue or the design note

  • Put the shared function in operation_span_status.py to keep status policy separate from exception event construction and each view's timestamp policy.
  • Call the existing exception recorder for every operation end; its existing None check suppresses absent diagnostics without another condition in each view.
  • Strengthen the existing empty-error test to check the moved message fallback; reuse lifecycle coverage rather than add duplicate direct-function tests.

Checklist

  • Each moved status branch has meaningful lifecycle coverage; branch mutation results are recorded above (rule 4).
  • This refactor preserves behavior; a failing bug reproduction is not applicable (rule 5).
  • Every edited test expectation is listed above with its reason (rule 5).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime November 1, 2026 03:29 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime November 1, 2026 03:30 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime November 2, 2026 20:37 — with GitHub Actions Active
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings after reviewing all five changed files against CONTRIBUTING rules 1–9. The extraction preserves status and exception behavior in both views. Tests were inspected but not executed; reported test and type-check results were not independently verified.

Reviewed commit 90e78a56c5f342a3c827ec2338ebc58f440fba9c. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — 90e78a56 Deployed Nov 2, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2735
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.

1 participant