fix(observed): correct enrichment and test-harness behavior - #712
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes several behavioral and ergonomics issues across the observed runtime and the observed_testing harness, with additional cleanup to examples/benchmarks and OpenTelemetry log-record mapping to remove deprecated metadata.
Changes:
- Optimize enrichment pushes so empty enrichment layers become true no-ops (avoiding unnecessary slot writes / guard work) and add coverage tests for the empty/merged/transfer cases.
- Make
EventProcessor::flushoptional via a default no-op implementation and remove redundantflushimplementations from examples. - Correct
observed_testing::ExpectedEventdefaults/docs to match exact-match behavior and makeExpectedEvent::newactually default to expecting a log signal; update docs/tests accordingly. - Remove deprecated
code.namespaceemission from the OTel mapping and update the integration test assertion. - Rename the sink-related benchmark group and identifiers to match what the benchmarks actually do.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/observed/tests/otel_log_record.rs | Updates OTel integration test to assert deprecated code.namespace is not exported. |
| crates/observed/src/sink/core.rs | Returns a no-op enrichment guard early when the enrichment entry set is empty. |
| crates/observed/src/processing/processor.rs | Provides a default no-op implementation for EventProcessor::flush. |
| crates/observed/src/enrichment/slot.rs | Adds empty-layer short-circuits in enrichment push paths, introduces Guard::empty, and adds coverage tests. |
| crates/observed/src/enrichment/mod.rs | Makes EnrichmentEntry publicly documented (removes #[doc(hidden)]). |
| crates/observed/src/enrichment/enrichment_trait.rs | Adds a manual Enrichment implementation example to docs. |
| crates/observed/examples/*.rs | Removes redundant flush implementations now covered by the trait default. |
| crates/observed/examples/support/otel.rs | Removes deprecated code.namespace attribute mapping from OTel log records. |
| crates/observed/examples/layered_app/token_issuer.rs | Uses shared taxonomy constant and switches failure metric to a monotonic counter. |
| crates/observed/benches/observed_benchmarks.rs | Renames sink-operation benchmark group and identifiers for accuracy. |
| crates/observed_testing/src/mock_processor.rs | Fixes ExpectedEvent defaults/docs to be exact-match and to expect log by default; adds a pinning test. |
| crates/observed_testing/src/lib.rs | Updates crate docs example to remove redundant .log() usage. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 587 587
Lines 62997 63002 +5
=======================================
+ Hits 62997 63002 +5
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:
|
b963994 to
41920bf
Compare
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 12 findings. No finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Hit |
41920bf to
5c5ae97
Compare
Rename sink benchmarks to match measured operations Use shared taxonomy and monotonic validation failure counter Skip empty enrichment layers before mutating slots Document manual enrichment entries and default no-op flush Correct expected-event log defaults and OTel source attributes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Roll back the two changes the crate owner rejected: - EventProcessor::flush goes back to a required method. A default no-op lets a processor that buffers telemetry silently skip flushing it, and losing telemetry is worse than the repeated Ok(()). - EnrichmentEntry returns to #[doc(hidden)] and the manual Enrichment implementation example is removed. The type is public only because Rust has no way to hide it from the derive expansion; the derive macro remains the single supported way to build an enrichment. Also drop the "composites with zero children" note from Sink::push_enrichment, which described an internal dispatch detail, and give EnrichmentTransfer::push_for the same empty-layer early return as push so a targeted layer with no entries allocates nothing either. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both callers of `EnrichmentTransfer::push_entries` already return early when the enrichment layer is empty, so the `entries.is_empty()` half of its guard can never be true. It decided nothing, which the mutation gate caught: replacing `||` with `&&` survived on all three platforms. The empty-layer no-op is unchanged — it is enforced where the layer is still owned, in `push` and `push_for`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`ExpectedEvent` held the log expectation twice: `severity: Option<Severity>` and a separate `expect_log` flag. A captured event carries a severity exactly when it carries a log signal, so the flag could express states no event can satisfy - `without_severity().log()` was unsatisfiable. Removes `expect_log` and the `log()` builder, drops the now-redundant log comparison, and updates the call sites. Also addresses review feedback: - Scopes the "exact-match" claim to what `ExpectedEvent` models, and says source location and metric instrument metadata are outside the comparison. - Uses "signal" and "flag" consistently with the crate's event model. - Notes at each empty-enrichment guard which cost it avoids, and that the entry slice is allocated by the caller of `Sink::push_enrichment`. - Records beside the OTel mapper why the emitting crate is not exported while file and line are. - Fixes the benchmark overview bullet left behind by the group rename. - Groups the `FieldlessLog` fixture with the imports and imports `SinkId`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A trailing comma after the second argument of `assert_eq!` reads like an empty custom-message argument. Both call sites take the two-argument form, so the comma is removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5c5ae97 to
befe9d6
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are targeted, internally consistent (empty-enrichment fast paths + updated test harness semantics), and are accompanied by updated tests covering the new behavior.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
Problem
Behavioral and ergonomics defects in
observedandobserved_testing, each confirmed againstmain. Unlike the sibling documentation pull requests, this one changes code.What changes
Runtime
Slot::push,Sink::push_enrichmentandEnrichmentTransferinstalled anEnrichmentNodeand touched every captured slot even when the enrichment layer was empty. An empty layer now returns a no-op guard and leaves the slot untouched.push_fortakes the same early return, so an empty targeted layer allocates nothing either. Guard merge and drop semantics are unchanged; new tests cover the empty push, the merged-guard case, and the transfer path.pushandpush_for, where the layer is still owned. The privateEnrichmentTransfer::push_entriesbriefly repeated the check, but by then both callers had already returned, so that half of its guard could never be true; the mutation gate caught it as a surviving||/&&mutant and it is removed.Vec, so they also skip the shared entry slice. TheSinkandSlotguards run after.enrich(...)has already built that slice, so they avoid composite fan-out, thread-local initialization and slot writes, but not the slice itself.Test harness
ExpectedEventstored the log expectation twice: once inseverity: Option<Severity>and once in a separateexpect_logflag. A captured event carries a severity exactly when it carries a log signal, so the flag added no valid state, andwithout_severity().log()was an expectation no event could satisfy.expect_logand thelog()builder are removed, the log expectation now follows the severity, and the.log()call sites go with them. A fieldless-log test pins the behavior.PartialEqcompares body, dimensions, metric and disabled exactly. The documentation now says so, and scopes the claim: source location and metric instrument metadata are outside the comparison, and are reached through theCapturedEventaccessors.observed_testingis not published, so this reaches no external consumer.Examples and benchmarks
support/otel.rsexported the emitting crate ascode.namespace, an attribute OpenTelemetry has deprecated. The mapping is removed, the test that pinned it now asserts absence, and a comment beside the mapper records why file and line stay while the crate does not.layered_app/token_issuer.rshand-builtDataClass::new("microsoft", "PublicNonPersonalData")instead of using the shared taxonomy constant, and recorded validation failures as an up-down counter carrying a constantfailure_count: 1. A failure count only increases, so it is now a fieldless counter.emit_contextbenchmark group named operations its bodies do not perform:attach_emitterconstructs and drops aSink, andemit_event_no_emitteremits throughSink::noop(). The group, functions, identifiers and the module overview are renamed to match. This changes benchmark identifiers, so historical series under the old names do not carry over.Withdrawn after review
Two changes were reverted in
b9639949:EventProcessor::flush. A default body lets a processor that buffers telemetry silently skip flushing it, with no error and no compile-time signal. The repeatedOk(())in stateless processors is the cheaper cost.flushstays a required method, and no processor code is touched by this pull request.EnrichmentEntry. The type is public only because Rust cannot hide a type the derive expansion must reference. It is not a stable construction API, and#[derive(Enrichment)]remains the single supported route, so it is back to#[doc(hidden)].Effects
Transferit also costs no entry-slice allocation;.enrich(...)and.enrich_for(...)still build theirArc<[EnrichmentEntry]>before the guard is reached.ExpectedEvent::newmatches its documentation andExpectedEvent::logno longer exists; assertions that relied on the previous default change meaning, and call sites that used.log()no longer compile.code.namespaceattribute.observedpublic surface, includingEventProcessorand theenrichmentre-exports, is as it was onmain. The removedExpectedEvent::logis a behavioral and signature change to the unpublishedobserved_testingharness contract.Validation
just anvil-fmt,just anvil-clippy,just anvil-readme-check,just anvil-spellcheck,cargo +1.96.1 check --workspace --all-targets,cargo +1.96.1 test -p observed -p observed_testing -p fetch_winhttp_impl, and acargo mutantsrun scoped tocrates/observed/src/enrichment/slot.rsall pass. The branch is rebased onmain.