Skip to content

feat(otel): add durable sampling coordination - #688

Merged
ayushiahjolia merged 1 commit into
mainfrom
otel-shared-trace-pr2-sampling
Sep 1, 2026
Merged

feat(otel): add durable sampling coordination#688
ayushiahjolia merged 1 commit into
mainfrom
otel-shared-trace-pr2-sampling

Conversation

@ayushiahjolia

@ayushiahjolia ayushiahjolia commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available: #674

Description of changes:

  • Second PR in the stack for reparenting durable OTel spans onto one shared execution trace.
  • Adds the sampling machinery that lets a single decision be resolved once per invocation and applied to every durable span. Not yet wired into the plugins, so behavior is unchanged.
  • Next in stack: wire both plugins onto the shared execution trace and sampling.

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

@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 00:14 — with GitHub Actions Inactive
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 00:14 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-trace-pr2-sampling branch from 429f004 to b37591b Compare August 29, 2026 02:04
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 02:17 — with GitHub Actions Inactive
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 02:17 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-trace-pr2-sampling branch from b37591b to daabf76 Compare August 29, 2026 03:18
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 03:19 — with GitHub Actions Inactive
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 03:19 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

One resource-lifetime issue found. Residual risk: OTel 1.20 compatibility is simulated rather than tested against that version.

Reviewed commit daabf76aeb9dc838dfed7d8b77a04d73d817fb21. Workflow run

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

This PR adds a self-contained OTel sampling-coordination module (durable_sampling.py) plus a thorough unit test suite. Per the description it is not yet wired into the plugins, so runtime behavior is unchanged.

I reviewed the sampling decision logic, the OTel version-compatibility shim, trace-state propagation, attribute merging, context-key storage, and the tests. The implementation is careful and well-covered:

  • resolve_sampling_result correctly prioritizes an explicit backend decision, then a same-trace ambient span, then delegates to the configured sampler with a fresh root context. trace_state is preserved only for same-trace ambient spans (matching the documented contract), and preserving it on DROP is intentional and correct for downstream propagation.
  • _delegate_accepts_trace_state / _delegate_should_sample use a signature probe instead of a call-time try/except, correctly avoiding masking a TypeError from a sampler body, and handle **kwargs-style and pre-1.21 (links-terminated) signatures. All branches are tested, including the raising-body case.
  • DurableSampler merges span/intent attributes (intent wins), preserves the intent's trace_state, and is idempotent on install. install_on_tracer mirrors the existing DeterministicIdGenerator.install_on_tracer pattern and correctly uses the public SDK Tracer.sampler attribute.

No blocking findings. No correctness, determinism, serialization, or concurrency defects warranting an inline comment were confirmed.

Residual test risk / minor, non-blocking observations:

  • The module is exercised only in isolation; plugin integration (parent-context intent storage, and consistency between resolve_sampling_result's UNDECIDED-on-canonical-trace path and execution_trace_context._trace_flags's root_sampled() path) is deferred to the next stacked PR and not yet integration-tested. See packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/durable_sampling.py:127.
  • _delegate_accepts_trace_state caches on the delegate's bound method via functools.lru_cache(maxsize=None), holding a strong reference to each distinct sampler for process lifetime and raising TypeError for an unhashable delegate; all standard OTel samplers are hashable, so this is theoretical (durable_sampling.py:152).
  • test_durable_sampler_merges_span_and_intent_attributes asserts the intent wins on a shared key but does not assert span-only keys survive the merge.

Reviewed commit daabf76aeb9dc838dfed7d8b77a04d73d817fb21. Workflow run

Base automatically changed from otel-shared-execution-trace to main August 31, 2026 17:13
@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-trace-pr2-sampling branch from daabf76 to d4c905f Compare August 31, 2026 17:13
@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-trace-pr2-sampling branch from d4c905f to 52a82a5 Compare August 31, 2026 17:50
@ayushiahjolia

Copy link
Copy Markdown
Contributor Author

/ai review

@ayushiahjolia
ayushiahjolia marked this pull request as ready for review August 31, 2026 18:24
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 31, 2026 18:24 — with GitHub Actions Inactive
@ayushiahjolia
ayushiahjolia merged commit 9052f35 into main Sep 1, 2026
18 of 28 checks passed
@ayushiahjolia
ayushiahjolia deleted the otel-shared-trace-pr2-sampling branch September 1, 2026 18:37
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.

2 participants