Skip to content

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

Draft
ayushiahjolia wants to merge 1 commit into
otel-shared-execution-tracefrom
otel-shared-trace-pr2-sampling
Draft

feat(otel): add durable sampling coordination#688
ayushiahjolia wants to merge 1 commit into
otel-shared-execution-tracefrom
otel-shared-trace-pr2-sampling

Conversation

@ayushiahjolia

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.

Depends only on the ExtractedContext/Sampling types from the previous PR.
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
parent_context,
trace_id,
name,
kind,

This comment was marked as outdated.

Comment on lines +152 to +153
at ``links``. A parameter probe (rather than a call-time ``try/except``)
avoids masking a ``TypeError`` raised inside the sampler body and never

This comment was marked as outdated.

@github-actions

This comment has been minimized.

Comment on lines +180 to +188
return sampler.should_sample(
parent_context,
trace_id,
name,
kind,
attributes,
links,
trace_state,
)

This comment was marked as outdated.

@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 deployed to ai-pr-review-runtime August 29, 2026 03:19 — with GitHub Actions Active
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 29, 2026 03:19 — with GitHub Actions Inactive
sampler,
Context(),
canonical_trace_id,
span_name,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex AI review

[P2] Avoid retaining delegate samplers in an unbounded global cache. The cache key is a bound should_sample method, which strongly references its sampler instance. With maxsize=None, every distinct sampler/provider used by a warm process remains reachable permanently. Remove the cache, make it bounded or weak, or cache a non-instance key; add a weak-reference lifecycle test.

@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

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