feat(observed): add event sampling support - #725
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API and implementation are cohesive, align with the documented contract, and are backed by targeted tests covering key semantics (interest gating, composites, cloning, and dyn events).
Pull request overview
Adds opt-in per-sink event sampling to the observed crate, allowing users to synchronously gate delivery to a sink’s processors based on a read-only event context (description, leaf sink id, and timestamp) while preserving borrowed-event emission behavior.
Changes:
- Introduces a new
observed::samplingmodule definingEventSampler,EventContext, andEventSamplingDecision. - Adds
Sink::with_event_sampler(...)and wires sampler decisions into leaf dispatch (single and composite sinks), withnoopremaining unaffected. - Adds integration tests in
observed_testingcovering sampling decisions, composite behavior, cloning/ownership semantics, and dynamic-event interop.
File summaries
| File | Description |
|---|---|
| crates/observed/src/sink/core.rs | Adds sampler storage to leaf sink state and applies sampling decisions during dispatch; exposes Sink::with_event_sampler. |
| crates/observed/src/sampling/mod.rs | Defines the public sampling API (context, decision enum, sampler trait) and documents the contract. |
| crates/observed/src/lib.rs | Exposes the new sampling module publicly. |
| crates/observed/FEATURES.md | Documents the new per-sink sampling capability at a feature level. |
| crates/observed/DESIGN.md | Updates sink/routing design documentation to include the sampling gate in the dispatch contract. |
| crates/observed_testing/tests/event_sampling.rs | Adds end-to-end tests validating sampling behavior across sink types and emission styles. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa28f79b-7be9-49b1-9b96-361080fad46c
f2525a2 to
ef503c8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The documented “exactly once” sampling call contract appears stricter than the actual dispatch behavior under the reentrancy guard, so the public docs should be tightened to match reality.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 582 583 +1
Lines 62852 62891 +39
=======================================
+ Hits 62852 62891 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🤖 Pato's Pull Request Agent: The routing flowchart in DESIGN.md (the |
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API, wiring, documentation updates, and test coverage align with the stated contract and are internally consistent across single/composite/noop sinks.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Fixed, thank you! |
There was a problem hiding this comment.
🟢 Approval recommended
The sampling contract is implemented at the leaf-dispatch boundary and is backed by targeted unit/integration tests covering the intended invocation and edge-case behavior.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
[AI AGENT]: Reviewing as an AI agent, with most attention on the new public sampling contract. I took this PR's green CI as the baseline rather than re-running it, and verified the two substantive findings below locally against c8062d44 with targeted probes.
Public surface reviewed: observed::sampling (EventContext, EventSamplingDecision, EventSampler) and Sink::with_event_sampler. One finding, on EventContext's constructor. The rest of the surface holds up: Arc<dyn EventSampler> matches the crate's existing Sink::new(.., Vec<Arc<dyn EventProcessor>>, ..) family and is what makes one sampler shareable across leaves; EventContext's private fields leave room to add inputs without a break; and an unsealed single-method trait is the right shape for an intended extension point. I did not re-raise the EventContext name or #[non_exhaustive] points already settled in earlier threads.
Correctness - no defects found. I traced every changed dispatch path. Two things worth recording as sound rather than assumed: the sampler runs inside the reentrancy guard taken in emit_impl (sink/core.rs L403), so a sampler that emits telemetry is dropped rather than recursing, which is what makes the "not supported" wording safe rather than merely discouraged; and with_event_sampler clones SingleSinkState, whose Slot is Arc<ThreadLocal<..>>, so leaf identity and the shared enrichment slot genuinely survive attachment exactly as the doc claims.
Tests: one gap, commented inline. Otherwise the suite is behaviour-shaped and covers the semantics I would want pinned - interest gating, composite independence, clone-before/clone-after, replacement, borrowed and DynEvent emission, and the noop/empty/processorless corners.
Performance, telemetry, resilience, dependencies: no findings. The added hot-path cost is one Option check plus one virtual call; the clock read that precedes it is contract, not waste, since EventContext::timestamp exposes it. No new dependencies, no new features, no telemetry emitted.
Verdict: approve with non-blocking comments. Nothing here blocks the merge. The composite test is the one item I would like to see land with the feature, since nothing else in CI can catch that regression.
Sergey Galkin (sgalkin)
left a comment
Sergey Galkin (sgalkin)
left a comment
There was a problem hiding this comment.
🤖 Co-authored with Copilot.
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API, sink integration, and documentation/tests are consistent with the stated per-leaf sampling contract and do not introduce observable correctness issues in the reviewed changes.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API and dispatch integration match the stated contract and are backed by focused unit/integration coverage across key sink configurations.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
Implementation and tests look coherent; only minor documentation wording issues were found (references to a crate-private module path).
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API and its integration are consistent with existing sink dispatch semantics and are backed by targeted unit/integration coverage for key behaviors (single/composite/noop, cloning, replacement, and dyn-event interop).
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API and sink integration are consistent with existing routing/interest behavior and are backed by targeted unit and integration tests without any evident correctness or design issues in the changed code paths.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The sampling API and dispatch integration are coherent with existing routing semantics, and the change includes targeted tests and documentation updates for the new behavior.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
40c705c
into
microsoft:main
Adds opt-in event sampling to
observed.Sinkis interested in.Sinks while preserving borrowed event emission.