fix(telemetry): keep event store spans tied to their destination

[yordis]

• EventStore spans currently group under the Commanded application, which makes store operations and subscription failures look like router failures in tracing backends.
• Using the configured event store as the destination keeps the EventStore spans aligned with messaging semantics and makes live traces easier to interpret.

[cursor]

PR Summary

Low Risk
Changes are limited to telemetry/span naming and attributes, plus a guarded config lookup; main risk is observability/dashboard churn if consumers relied on the old span names.

Overview
Event store OpenTelemetry instrumentation now resolves the configured event store identifier and records it as messaging.destination.name, using it (when available) to suffix span names (e.g. append_to_stream <store>), rather than grouping spans under the Commanded application.

Destination lookup is made defensive: failures to fetch adapter metadata are caught, a [:commanded, :opentelemetry, :warning] telemetry event is emitted, and spans proceed without a destination; tests are updated accordingly and new cases cover malformed/missing config and ensure handlers remain attached on errors.

^{Reviewed by Cursor Bugbot for commit 239d6e5. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Warning

Rate limit exceeded

@yordis has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 34 minutes and 6 seconds before requesting another review.

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 34 minutes and 6 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: edab8069-70d7-4932-b717-c14460d1ff64

📥 Commits

Reviewing files that changed from the base of the PR and between b960596 and 239d6e5.

📒 Files selected for processing (2)

• lib/commanded/opentelemetry/event_store.ex
• test/opentelemetry/event_store_test.exs

Walkthrough

EventStore OpenTelemetry instrumentation now resolves a destination name from the configured event-store adapter and, when present, adds it as messaging.destination.name and prefers it in span names. Adapter metadata lookup is guarded; failures emit a telemetry warning and suppress destination-derived attributes/naming.

Changes

Cohort / File(s)	Summary
Event Store OpenTelemetry Instrumentation lib/commanded/opentelemetry/event_store.ex	Add destination resolution helpers (event_store_destination_name/1, lookup_event_store_name/1, to_destination_name/1); wrap adapter lookup in rescue to emit [:commanded, :opentelemetry, :warning] on error; include messaging.destination.name attribute and prefer destination when building span names.
Event Store Instrumentation Tests test/opentelemetry/event_store_test.exs	Update tests to derive destination_name via adapter metadata, assert messaging.destination.name across operations, adjust span name expectations, replace synthetic stop/exception telemetry tests with defensive cases capturing emitted telemetry warnings and error attributes, and add helpers for warning capture and destination computation.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant EventStore as Commanded.EventStore
  participant AppCfg as CommandedApplication
  participant Instr as Opentelemetry.EventStore.Instrumentation
  participant OTel as OpenTelemetry/Exporter

  Client->>EventStore: perform operation (append/subscribe/ack/...)
  EventStore-->>Instr: emit telemetry event (start/stop/exception)
  Instr->>AppCfg: CommandedApplication.event_store_adapter(application)
  AppCfg-->>Instr: adapter metadata (or raise / nil)
  Instr->>Instr: resolve destination_name (lookup_event_store_name -> to_destination_name)
  alt adapter lookup succeeded
    Instr->>OTel: start span (name includes destination_name)
    Instr->>OTel: set attributes including "messaging.destination.name"
  else adapter lookup failed
    Instr->>Instr: emit [:commanded, :opentelemetry, :warning] telemetry
    Instr->>OTel: start span (name uses action only)
  end
  Instr->>OTel: end span
  OTel-->>Client: span exported

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat: add OpenTelemetry EventStore instrumentation #37: earlier changes to EventStore OpenTelemetry instrumentation that introduced span naming/attribute logic refined here.
• test(telemetry): keep EventStore span assertions anchored in runtime behavior #84: touches the same EventStore instrumentation and tests; closely related assertions and runtime-span testing.

Poem

🐰 I hopped through adapters, sniffed a name,
Found destinations in metadata's game.
I stamped each span with where it goes,
A twitch, a hop — telemetry glows! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: keeping event store spans tied to their destination instead of grouping them under the application.
Description check	✅ Passed	The description explains the problem (EventStore spans grouping under the application causing confusion) and the solution (using the configured event store as the destination).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch yordis/fix-event-store-span-destination

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

test/opentelemetry/event_store_test.exs (1)

473-483: Internal otel structure access is fragile but acceptable for tests.

The pattern matching on {:attributes, _, _, _, attrs_map} relies on the internal representation of OpenTelemetry attributes. Consider adding a brief comment noting this dependency, so future maintainers know to update this if otel library internals change.

🤖 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 473 - 483, The test
helper assert_exception_event relies on OpenTelemetry's internal event/attribute
tuple shape (it pattern matches the result of :otel_events.list and specifically
{:attributes, _, _, _, attrs_map} to extract attributes); add a brief
explanatory comment above the assert_exception_event function that calls out
this fragile dependency on otel internals and instructs maintainers to update
this pattern if the otel library changes its internal representation — reference
the function name assert_exception_event and the tuple pattern {:attributes, _,
_, _, attrs_map} in the comment so it's clear what to watch for.

lib/commanded/opentelemetry/event_store.ex (1)

209-216: Good defensive error handling for adapter lookup.

The rescue clauses cover the expected failure modes when querying adapter configuration. One consideration: if the adapter contract changes and event_store_adapter/1 could raise other exception types (e.g., FunctionClauseError), they would propagate and potentially crash the telemetry handler.

Consider whether a broader catch might be warranted for maximum resilience:

rescue
  _ -> nil

However, the current explicit list is also valid if you prefer to fail-fast on unexpected errors during destination lookup.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/commanded/opentelemetry/event_store.ex` around lines 209 - 216, The
current fetch_event_store_adapter_meta function rescues only ArgumentError,
MatchError and RuntimeError from
CommandedApplication.event_store_adapter(application); broaden the rescue to
catch all exceptions so unexpected errors (e.g., FunctionClauseError) won't
crash the telemetry handler by replacing the explicit list with a generic rescue
(e.g., rescue _ -> nil) in fetch_event_store_adapter_meta to return nil for any
raised exception from CommandedApplication.event_store_adapter/1.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lib/commanded/opentelemetry/event_store.ex`:
- Around line 209-216: The current fetch_event_store_adapter_meta function
rescues only ArgumentError, MatchError and RuntimeError from
CommandedApplication.event_store_adapter(application); broaden the rescue to
catch all exceptions so unexpected errors (e.g., FunctionClauseError) won't
crash the telemetry handler by replacing the explicit list with a generic rescue
(e.g., rescue _ -> nil) in fetch_event_store_adapter_meta to return nil for any
raised exception from CommandedApplication.event_store_adapter/1.

In `@test/opentelemetry/event_store_test.exs`:
- Around line 473-483: The test helper assert_exception_event relies on
OpenTelemetry's internal event/attribute tuple shape (it pattern matches the
result of :otel_events.list and specifically {:attributes, _, _, _, attrs_map}
to extract attributes); add a brief explanatory comment above the
assert_exception_event function that calls out this fragile dependency on otel
internals and instructs maintainers to update this pattern if the otel library
changes its internal representation — reference the function name
assert_exception_event and the tuple pattern {:attributes, _, _, _, attrs_map}
in the comment so it's clear what to watch for.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab15b7d7-dbd9-481a-a75d-3b61d0a3532d

📥 Commits

Reviewing files that changed from the base of the PR and between 6c0cb3d and 1fc61dd.

📒 Files selected for processing (2)

• lib/commanded/opentelemetry/event_store.ex
• test/opentelemetry/event_store_test.exs

[cursor]

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 70adaf5. Configure here.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/opentelemetry/event_store_test.exs (1)

386-433: ⚠️ Potential issue | 🟡 Minor

Restore DefaultApp config after these mutations.

Line 386 and Line 429 overwrite DefaultApp's :event_store config and never restore it. That makes the module order-dependent: any later test that boots or uses DefaultApp can inherit nil or :not_a_map instead of the real adapter. Please snapshot the original value and put it back in on_exit.

🤖 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 386 - 433, Before
mutating DefaultApp's :event_store via AppConfig.__put__, capture the original
value (e.g. orig = AppConfig.get(DefaultApp, :event_store) or
AppConfig.__get__(DefaultApp, :event_store)); then perform the AppConfig.__put__
calls as currently written and register an on_exit callback that restores the
original with AppConfig.__put__(DefaultApp, :event_store, orig). Do this for the
tests that change DefaultApp (the test that calls AppConfig.__put__(DefaultApp,
:event_store, nil) and the test that sets it to
{Commanded.EventStore.Adapters.InMemory, :not_a_map}) so the DefaultApp config
is always restored after each test.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/opentelemetry/event_store_test.exs`:
- Around line 463-467: The helper expected_event_store_destination/1 is forcing
inspect/1 on the adapter destination which turns binaries into quoted strings
and nil into "nil"; update expected_event_store_destination(application) to
mirror the runtime normalization used by
lib/commanded/opentelemetry/event_store.ex by returning destination_name as-is
when it's a binary, returning nil when destination_name is nil, and only falling
back to inspect(destination_name) for other types; locate the function
(expected_event_store_destination) and replace the single
inspect(destination_name) call with a conditional that checks
is_binary(destination_name) and is_nil(destination_name).

---

Outside diff comments:
In `@test/opentelemetry/event_store_test.exs`:
- Around line 386-433: Before mutating DefaultApp's :event_store via
AppConfig.__put__, capture the original value (e.g. orig =
AppConfig.get(DefaultApp, :event_store) or AppConfig.__get__(DefaultApp,
:event_store)); then perform the AppConfig.__put__ calls as currently written
and register an on_exit callback that restores the original with
AppConfig.__put__(DefaultApp, :event_store, orig). Do this for the tests that
change DefaultApp (the test that calls AppConfig.__put__(DefaultApp,
:event_store, nil) and the test that sets it to
{Commanded.EventStore.Adapters.InMemory, :not_a_map}) so the DefaultApp config
is always restored after each test.

🪄 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: bf778ac0-eb05-405e-be28-7f65b54d9941

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc61dd and b960596.

📒 Files selected for processing (2)

• lib/commanded/opentelemetry/event_store.ex
• test/opentelemetry/event_store_test.exs

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/commanded/opentelemetry/event_store.ex