fix(telemetry): enrich EventStore spans with full OTel semantic attributes - #92
Conversation
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit fdf1b3c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThe PR enhances OpenTelemetry instrumentation for Commanded's event store by adding detailed span attributes for streaming operations, extracting attribute-construction logic into reusable helpers, and reformatting exception type reporting to remove the "Elixir." prefix convention. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
b71eccb to
1e6f2db
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/commanded/opentelemetry/commanded_attributes.ex (1)
188-201: Align these new attribute names with the module contract.The module docs say all attributes
MUSTuse thecommanded.prefix, but these new public constants returneventstore.*. Either rename them to the Commanded namespace or update the docs to explicitly carve out EventStore-specific attributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/opentelemetry/commanded_attributes.ex` around lines 188 - 201, The public attribute functions eventstore_read_count, eventstore_stream_start_version, eventstore_stream_direction, eventstore_stream_batch_size, and eventstore_stream_delete_type currently return atoms under the "eventstore.*" namespace which violates the module contract that all attributes MUST use the "commanded." prefix; update each function and its `@spec` to return the corresponding "commanded.eventstore.*" atom (e.g. :"commanded.eventstore.read.count", etc.) so they conform to the module naming convention, and adjust any callers/tests or the module docs if you instead decide to explicitly declare these as EventStore-specific (but prefer renaming for consistency).test/opentelemetry/event_store_test.exs (1)
94-96: LGTM — new attribute assertions match the enriched span schema.
commanded.event.count,eventstore.stream.start_version,eventstore.stream.batch_size, anddb.systemare consistently asserted across success, missing-stream, and error/exception paths.One note: the
stream_forwardassertion hardcodes"eventstore.stream.batch_size": 1000. If the default batch size is sourced from config and ever changes, this test will be brittle. Consider deriving from the same constant/config the production code uses.Also applies to: 120-123, 219-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/event_store_test.exs` around lines 94 - 96, The test hardcodes "eventstore.stream.batch_size": 1000 in the stream_forward assertion which will break if the production default changes; update the test to derive the expected batch size from the same source the runtime uses (e.g., call the production accessor or read the application config) instead of the literal 1000 so the assertion in the stream_forward test (and the other occurrences at the referenced assertion blocks) stays in sync with the production constant/config.
🤖 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/opentelemetry/helpers.ex`:
- Around line 50-55: The fallback atom clause returns "Elixir.RuntimeError" for
module atoms; add a specific clause in Helpers.to_error_type to handle module
atoms before the generic atom fallback: add a clause like def
to_error_type(module, _tracer_id) when is_atom(module) and
String.starts_with?(Atom.to_string(module), "Elixir."), do: inspect(module)
(placed above def to_error_type(error, _tracer_id) when is_atom(error)), so
module atoms use inspect/1 while non-module atoms (e.g., :validation_failed)
still fall through to to_string/1.
In `@test/support/opentelemetry_case.ex`:
- Around line 61-69: The cleanup is generating telemetry names as [:eventstore,
op, suffix] but Commanded.OpenTelemetry.EventStore.setup/0 registers handlers
under [:commanded, :event_store, op, suffix], so update the eventstore_events
construction (which currently builds eventstore_events from
eventstore_operations and suffixes) to produce [:commanded, :event_store, op,
suffix] instead, ensuring the loop that iterates for event <- commanded_events
++ eventstore_events will detach the exact handlers registered by
Commanded.OpenTelemetry.EventStore.setup/0.
---
Nitpick comments:
In `@lib/commanded/opentelemetry/commanded_attributes.ex`:
- Around line 188-201: The public attribute functions eventstore_read_count,
eventstore_stream_start_version, eventstore_stream_direction,
eventstore_stream_batch_size, and eventstore_stream_delete_type currently return
atoms under the "eventstore.*" namespace which violates the module contract that
all attributes MUST use the "commanded." prefix; update each function and its
`@spec` to return the corresponding "commanded.eventstore.*" atom (e.g.
:"commanded.eventstore.read.count", etc.) so they conform to the module naming
convention, and adjust any callers/tests or the module docs if you instead
decide to explicitly declare these as EventStore-specific (but prefer renaming
for consistency).
In `@test/opentelemetry/event_store_test.exs`:
- Around line 94-96: The test hardcodes "eventstore.stream.batch_size": 1000 in
the stream_forward assertion which will break if the production default changes;
update the test to derive the expected batch size from the same source the
runtime uses (e.g., call the production accessor or read the application config)
instead of the literal 1000 so the assertion in the stream_forward test (and the
other occurrences at the referenced assertion blocks) stays in sync with the
production constant/config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd16093c-cbeb-4bfd-9d8b-9b829fd539b5
📒 Files selected for processing (10)
lib/commanded/event_store.exlib/commanded/opentelemetry/commanded_attributes.exlib/commanded/opentelemetry/event_store.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exstest/opentelemetry/event_handler_test.exstest/opentelemetry/event_store_test.exstest/support/factory.extest/support/opentelemetry_case.ex
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
3fbc2ae to
b0441c2
Compare
…butes - Orphaned operations (ack_event, subscribe, subscribe_to, unsubscribe, delete_subscription) removed from EventStore instrumentation since they produce noise without meaningful tracing value - error.type now uses inspect() to drop the Elixir. prefix for cleaner APM display - Connection attributes (server.address, server.port, db.namespace) and peer.service set to the actual database name for proper service mapping - db.system derived from adapter type (postgresql for EventStore adapter, in_memory for InMemory) - event_count, start_version, read_batch_size added to span attributes - Shared attribute helpers extracted to Helpers module for reuse Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
b0441c2 to
fdf1b3c
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.
Reviewed by Cursor Bugbot for commit fdf1b3c. Configure here.
1 participant
ack_event,subscribe,subscribe_to,unsubscribe, anddelete_subscriptionwere removed from EventStore instrumentation because they are lifecycle/control-plane operations — they don't carry domain data and their spans add noise without actionable tracing value (e.g.,ack_eventfires on every processed event,subscribe/unsubscribeare one-shot setup calls). The remaining operations (append_to_stream,stream_forward,read_snapshot,record_snapshot,delete_snapshot) are the ones that actually touch persisted data and benefit from span-level observabilityerror.typedisplayedElixir.RuntimeErrorinstead ofRuntimeErrorin APM tools due toto_string()on module atomsserver.address,server.port,db.namespace,peer.service) needed for proper service topology mapping in Datadogdb.systemwas not set, preventing APM tools from categorizing spans by database technologyevent_count,start_version, andread_batch_sizewere missing from span attributes, reducing observability into EventStore operations