fix: emit populate telemetry for new aggregates (stream_not_found) - #58
Conversation
PR SummaryLow Risk Overview Updates OpenTelemetry instrumentation ( Written by Cursor Bugbot for commit 352c285. This will update automatically on new commits. Configure here. |
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds aggregate "load" telemetry around event-store streaming and applies events to rebuild state; instruments OpenTelemetry with load start/stop handlers and updates internal builder functions to return event counts for instrumentation. Public APIs remain unchanged. Changes
Sequence DiagramsequenceDiagram
participant ASB as Aggregate State Builder
participant TM as Telemetry System
participant OTH as OpenTelemetry Handlers
participant ES as Event Store
ASB->>TM: emit [:commanded, :aggregate, :load, :start]
TM->>OTH: handle telemetry start
OTH->>OTH: start span (commanded.aggregate.load)
ASB->>ES: stream_forward (events)
ES-->>ASB: stream or :stream_not_found
ASB->>ASB: rebuild_from_event_stream (apply events) -> {state, count}
ASB->>TM: emit [:commanded, :aggregate, :load, :stop] (count)
TM->>OTH: handle telemetry stop
OTH->>OTH: set span attrs (count, version) and end span
ASB-->>Caller: return state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f5d9ced to
7604dfa
Compare
c05656c to
303a30d
Compare
88e9764 to
de9c8b3
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
020564e to
499fbab
Compare
Move populate telemetry to wrap the entire rebuild_from_events flow so it fires for both stream_not_found (new aggregates) and event consumption (existing aggregates). Previously it only fired when events were consumed, leaving a gap in traces for new aggregate commands. Add tests for both paths: new aggregate (count: 0) and reload (count: N). Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/commanded/aggregates/aggregate_state_builder.ex`:
- Around line 93-121: When EventStore.stream_forward returns {:error,
:stream_not_found} the populate telemetry is never started because populate is
begun inside rebuild_from_event_stream/2; update the {:error, :stream_not_found}
branch in the load logic so it still invokes the populate telemetry path (either
by calling rebuild_from_event_stream/2 with an empty event stream or by
explicitly starting/stopping the populate telemetry/span with count: 0) so that
[:commanded, :aggregate, :populate, *] is emitted for new aggregates; reference
the load_prefix/load_start telemetry variables and the
rebuild_from_event_stream/2 helper when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68ec80e3-48de-44e0-9576-ce318690305e
📒 Files selected for processing (5)
lib/commanded/aggregates/aggregate_state_builder.exlib/commanded/opentelemetry/aggregate_populate.extest/aggregates/aggregate_telemetry_test.exstest/opentelemetry/aggregate_populate_test.exstest/support/opentelemetry_case.ex

Summary
Emit
commanded.aggregate.populatetelemetry for all aggregate loads, including new aggregates wherestream_forwardreturns{:error, :stream_not_found}. Previously the span only fired when events were consumed, leaving a gap in traces for new aggregate commands.Changes
rebuild_from_event_streamto wrap the entirerebuild_from_eventsflowstream_not_found: span fires withcount: 0(captures stream lookup latency)count: N(unchanged behavior)Testing