Repository navigation
fix(otel): export sampled fallback roots early - #754
zhongkechen wants to merge 14 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.
…/python-754-20261006
…/python-754-20261006
| concurrency: | ||
| group: cloud-tests-${{ matrix.python-prefix }} | ||
| cancel-in-progress: false | ||
| queue: max |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Checked the current official Actions workflow schema: concurrency-mapping supports queue, whose allowed values are single and max. Both workflow-level and job-level concurrency use this mapping.
The reviewed head 1a57d1d has also completed Cloud tests and Conformance Tests successfully with these declarations, so the workflows are accepted and running. Keeping queue: max together with cancel-in-progress: false retains pending runs while preserving the active tests.
| attributes={ | ||
| "durable.execution.arn": self.execution_arn, | ||
| "durable.execution.synthetic_root": True, | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed and fixed in dc429fd. The synthetic root now uses its own sampling intent with the SDK-owned execution ARN and synthetic-root marker taking precedence. Other sampler attributes, the resolved decision, and tracestate are preserved; the invocation's original sampling result is not mutated or resampled.
The regression reproduces both collisions before the fix for both plugin views, covering the first invocation and resume. It checks attributes as observed by a processor's on_start and by the exporter, stable root IDs, retained sampler metadata/tracestate, and one sampler call per invocation. All 387 OTel tests, package type checks, and lint/format checks pass.
This comment has been minimized.
This comment has been minimized.
| concurrency: | ||
| group: cloud-tests-${{ matrix.python-prefix }} | ||
| cancel-in-progress: false | ||
| queue: max |
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.
| sampling_intent: DurableSamplingIntent, | ||
| ) -> None: | ||
| """Export a sampled local ancestor; never replace an external parent.""" | ||
| if self.ancestor.is_remote or not self.ancestor.trace_flags.sampled: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Checked the published 1.20.0 release: TraceFlags.sampled is already defined as return bool(self & TraceFlags.SAMPLED). The official API 1.20.0 wheel contains the same property, and this code constructs the local ancestor's flags as TraceFlags instances.
I also installed both opentelemetry-api==1.20.0 and opentelemetry-sdk==1.20.0 and ran the complete OTel suite on 15caff4: all 449 tests pass, including fallback-root export in both views and decorated-handler suspend/resume cases. The existing accessor works at the declared dependency floor.
This comment has been minimized.
This comment has been minimized.
| sampling_intent: DurableSamplingIntent, | ||
| ) -> None: | ||
| """Export a sampled local ancestor; never replace an external parent.""" | ||
| if self.ancestor.is_remote or not self.ancestor.trace_flags.sampled: |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_bejdvhjdvhzsvsmin6hgyhxopm
P1: The standalone extra still supports OpenTelemetry 1.20, whose TraceFlags lacks .sampled. Every fallback invocation on that supported version raises AttributeError here, leaving the plugin with missing or fragmented telemetry. Use the existing version-compatible bitmask check with TraceFlags.SAMPLED, and add minimum-version coverage.
Codex AI reviewOne P1 compatibility regression remains in the fallback-root path; minimum-version OpenTelemetry coverage is missing. Reviewed commit |
Fixes #748, including the first-invocation timing requirement in aws/aws-durable-execution-sdk-js#938.
Both OTel views now export
DurableExecutionRootwhen a sampled invocation resolves an SDK-owned fallback ancestor. The anchor is materialized at invocation start and flushed through normal invocation end, includingPENDINGandRETRY, so an execution that times out while suspended already has its ancestor. Complete remote parents are untouched, and unsampled invocations emit no synthetic root.The span uses the existing ARN-scoped synthetic-parent ID,
durable.execution.synthetic_root=true, and the stable execution-start timestamp for both start and end. Each sampled fallback invocation can re-export it to recover lost telemetry; first-invocation retries retain the same SDK-owned identity/timing/attributes.Workflowcontinues to carry duration and terminal outcome. The synthetic-root path uses a private one-span ID scope, consumed before processors run, so reentrant roots on the same cached tracer get independent IDs. The exported generator's existinguse_idssignature and scope-wide trace override semantics remain unchanged. The normal configured tracer, provider resource, processors, and resolved sampler metadata are preserved. Provider resources/custom enrichment may vary across environments; this does not replace configured service identity or promise exactly-once export.Validation: after integrating main's view-registration guard, all 1,765 core tests (+5 subtests, 98.28% coverage) and 449 OTel tests pass. Fourteen valid registration/wait-resume cases pass against installed core 2.0.1 with site-packages imports verified. Repository type checks, core/OTel Hatch lint/format checks, workflow wiring and diff checks pass. The complete OTel suite exercises the workspace core; the installed legacy lane covers valid registrations because exclusivity validation requires core 2.1+. The runtime dependency floor remains core 2.0.0.
Regression evidence includes both views across absent/invalid/partial remote context, explicit and local sampling (including RECORD_ONLY), shared trace IDs, retry/resume, resource changes and buffered flushes. Real decorated handlers cover suspension/resume success and failure, skipped completed steps and timeout without a terminal hook. Ten lifecycle regressions failed without anchor materialization; the reviewer's 16-case guarded-processor probe and six resource/custom-processor/noncopyable-tracer probes passed after the fixes. Sampler-supplied attributes cannot replace the SDK's root identity before span processors run. No cloud deployment was run locally.