feat(otel): add durable sampling coordination - #688
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
429f004 to
b37591b
Compare
| parent_context, | ||
| trace_id, | ||
| name, | ||
| kind, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
| return sampler.should_sample( | ||
| parent_context, | ||
| trace_id, | ||
| name, | ||
| kind, | ||
| attributes, | ||
| links, | ||
| trace_state, | ||
| ) |
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.
b37591b to
daabf76
Compare
| sampler, | ||
| Context(), | ||
| canonical_trace_id, | ||
| span_name, |
There was a problem hiding this comment.
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.
Codex AI reviewOne resource-lifetime issue found. Residual risk: OTel 1.20 compatibility is simulated rather than tested against that version. Reviewed commit |
Claude AI reviewThis PR adds a self-contained OTel sampling-coordination module ( 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:
No blocking findings. No correctness, determinism, serialization, or concurrency defects warranting an inline comment were confirmed. Residual test risk / minor, non-blocking observations:
Reviewed commit |
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.