feat(telemetry): add OpenTelemetry instrumentation for eventstore library events - #89
feat(telemetry): add OpenTelemetry instrumentation for eventstore library events#89yordis wants to merge 1 commit into
Conversation
PR SummaryMedium Risk Overview Extends Reviewed by Cursor Bugbot for commit e95b7ef. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR extends OpenTelemetry instrumentation for EventStore operations in the Commanded framework. It introduces a new EventStore adapter module for telemetry handler management, updates configuration schema to support adapter-level event store tracing, adds helper functions for span attributes, refactors telemetry event handling, and updates test coverage with assertion corrections and comprehensive integration tests. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant ES as EventStore<br/>Library
participant ESAdapt as EventStore<br/>Adapter Handler
participant OTel as OpenTelemetry
App->>ES: Invoke operation<br/>(e.g., append_to_stream)
ES->>ESAdapt: Emit [:eventstore, action, :start]<br/>telemetry event
ESAdapt->>ESAdapt: Resolve span name & attributes<br/>from metadata
ESAdapt->>OTel: Start client-kind<br/>telemetry span
ES->>ES: Execute operation
ES->>ESAdapt: Emit [:eventstore, action, :stop]<br/>or :exception event
ESAdapt->>ESAdapt: Build span attributes<br/>(semantics, error type, etc.)
ESAdapt->>OTel: Record attributes & status
ESAdapt->>OTel: End telemetry span
OTel-->>App: Span recorded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/opentelemetry/eventstore_test.exs (1)
13-26: Consider centralizing the EventStore operation list.The test duplicates
EventStoreLib’s private operation list, so future operation additions can silently drift between instrumentation, cleanup, and coverage. Exposing a smallevents/0helper for tests or sharing a module attribute through a test helper would keep this in sync.Also applies to: 503-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/eventstore_test.exs` around lines 13 - 26, The test duplicates EventStoreLib's private operation list; add a single shared source (either make EventStoreLib expose a small public events/0 function returning the operation atom list or add a test helper module attribute/events/0 that delegates to EventStoreLib) and update the test's `@events` to call that helper instead of hardcoding the list (reference EventStoreLib and the new events/0 helper to locate where to add the shared list and where to replace the duplicated `@events`).
🤖 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/support/opentelemetry_case.ex`:
- Around line 41-43: The on_exit callback currently only restores
OTEL_TRACES_EXPORTER when prev_exporter is truthy; change it to restore or
remove the variable based on prev_exporter: if prev_exporter is non-nil call
System.put_env("OTEL_TRACES_EXPORTER", prev_exporter), otherwise call
System.delete_env("OTEL_TRACES_EXPORTER") so the test cleans up the environment
when the variable was originally unset; locate and update the on_exit block that
references prev_exporter and System.put_env to implement this conditional
restore/delete logic.
---
Nitpick comments:
In `@test/opentelemetry/eventstore_test.exs`:
- Around line 13-26: The test duplicates EventStoreLib's private operation list;
add a single shared source (either make EventStoreLib expose a small public
events/0 function returning the operation atom list or add a test helper module
attribute/events/0 that delegates to EventStoreLib) and update the test's
`@events` to call that helper instead of hardcoding the list (reference
EventStoreLib and the new events/0 helper to locate where to add the shared list
and where to replace the duplicated `@events`).
🪄 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: 0822a9bf-1dd0-4ec7-8b15-0ed8449fd722
📒 Files selected for processing (4)
lib/commanded/opentelemetry.exlib/commanded/opentelemetry/event_store_lib.extest/opentelemetry/eventstore_test.exstest/support/opentelemetry_case.ex
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 8a9af9b. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/commanded/opentelemetry/commanded_attributes.ex (1)
205-252: Optional: hoist theMessagingAttributesalias to the top of the module.The alias on line 207 is declared mid-module, immediately before its first use. Per common Elixir convention and for consistency with the other OTel modules in this PR (e.g.
event_store.ex,event_store/adapters/event_store.ex), consider moving it to the top alongside a module-level alias block.♻️ Proposed relocation
defmodule Commanded.OpenTelemetry.CommandedAttributes do `@moduledoc` """ ... """ + + alias OpenTelemetry.SemConv.Incubating.MessagingAttributes @@ # -- Span builder helpers -- - alias OpenTelemetry.SemConv.Incubating.MessagingAttributes - def maybe_add_operation_type(attrs, nil), do: attrs🤖 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 205 - 252, The alias OpenTelemetry.SemConv.Incubating.MessagingAttributes is declared mid-module right before its first use; move that alias declaration up into the module-level alias block alongside the other aliases (so functions like maybe_add_operation_type/2, maybe_add_destination_name/2, and maybe_add_subscription_name/2 can reference MessagingAttributes from the top), ensuring no other code semantics change.lib/commanded/opentelemetry/event_store.ex (1)
38-40: Nit: hoistalias Commanded.OpenTelemetry.EventStore.Adaptersto the module alias block.The alias is declared mid-module, after
@eventsand right beforesetup/1. Moving it up with the other aliases (lines 4–10) keeps the module header consistent and matches the convention used elsewhere in this PR.♻️ Proposed relocation
alias Commanded.Application, as: CommandedApplication alias Commanded.OpenTelemetry.CommandedAttributes + alias Commanded.OpenTelemetry.EventStore.Adapters alias Commanded.OpenTelemetry.Helpers alias OpenTelemetry.SemConv.ErrorAttributes alias OpenTelemetry.SemConv.Incubating.CodeAttributes alias OpenTelemetry.SemConv.Incubating.MessagingAttributes alias OpenTelemetry.Span @@ )a - alias Commanded.OpenTelemetry.EventStore.Adapters - def setup(config \\ []) do🤖 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 38 - 40, Move the mid-module alias declaration for Adapters into the existing module alias block to keep header consistency: relocate the line alias Commanded.OpenTelemetry.EventStore.Adapters so it appears alongside the other alias declarations at the top of the module (before the module attributes like `@events`), leaving the setup/1 function and `@events` unchanged.
🤖 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/commanded_attributes.ex`:
- Around line 205-252: The alias
OpenTelemetry.SemConv.Incubating.MessagingAttributes is declared mid-module
right before its first use; move that alias declaration up into the module-level
alias block alongside the other aliases (so functions like
maybe_add_operation_type/2, maybe_add_destination_name/2, and
maybe_add_subscription_name/2 can reference MessagingAttributes from the top),
ensuring no other code semantics change.
In `@lib/commanded/opentelemetry/event_store.ex`:
- Around line 38-40: Move the mid-module alias declaration for Adapters into the
existing module alias block to keep header consistency: relocate the line alias
Commanded.OpenTelemetry.EventStore.Adapters so it appears alongside the other
alias declarations at the top of the module (before the module attributes like
`@events`), leaving the setup/1 function and `@events` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f698b2f0-64c4-44a9-83f4-10d4dc77ef53
📒 Files selected for processing (10)
lib/commanded/opentelemetry.exlib/commanded/opentelemetry/commanded_attributes.exlib/commanded/opentelemetry/event_store.exlib/commanded/opentelemetry/event_store/adapters/event_store.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exstest/opentelemetry/event_handler_test.exstest/opentelemetry/event_store/adapters/event_store_test.exstest/opentelemetry/event_store_test.exs
✅ Files skipped from review due to trivial changes (1)
- test/opentelemetry/event_handler_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/commanded/opentelemetry.ex
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/commanded/aggregates/aggregate_state_builder.ex (1)
193-210:⚠️ Potential issue | 🟡 MinorUpdate the telemetry metadata docs for
caller_metadata.The emitted load/populate metadata now includes
caller_metadata, but thetelemetry_eventmetadata specs above still omit it. SinceTelemetryRegistryfeeds public telemetry docs, this new field should be documented for the affected load and populate events.📝 Proposed docs update
%{application: Commanded.Application.t(), aggregate_module: module(), aggregate_uuid: String.t(), aggregate_state: struct(), - aggregate_version: non_neg_integer()} + aggregate_version: non_neg_integer(), + caller_metadata: map()}Apply the same addition to the load
:start/:stopand populate:start/:stopmetadata specs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/aggregates/aggregate_state_builder.ex` around lines 193 - 210, The telemetry metadata docs are missing the new caller_metadata field; update the telemetry event metadata specs for the load and populate events (load :start, load :stop, populate :start, populate :stop) to include caller_metadata so TelemetryRegistry exposes it; locate the telemetry_metadata function and the telemetry_event/metadata specs for the load/populate events and add caller_metadata to their maps/typed specs consistently with how telemetry_metadata(%Aggregate{}) now returns it.
🤖 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/aggregate_populate.ex`:
- Around line 33-36: The :load :start handler in aggregate_populate.ex attaches
the propagated OTel context via
Helpers.extract_propagated_ctx(...)/:otel_ctx.attach(ctx) but the attached
context is never cleared, causing later spans in the aggregate GenServer to be
incorrectly parented; update the corresponding :load :stop telemetry handler
(the handler that ends the load span) to call Helpers.clear_ctx() immediately
after ending the span so the process restores a clean OTel context state. Ensure
you modify the :load :stop branch in the same module (look for the telemetry
stop handler that pairs with the :load :start logic) and add a call to
Helpers.clear_ctx() right after the span finish/cleanup.
In `@lib/commanded/opentelemetry/event_store/adapters/event_store.ex`:
- Around line 14-27: The `@events` list currently includes subscribe_to_stream and
delete_subscription which can produce orphan root spans from async EventStore
background subscription processes; either verify these telemetry events are only
emitted synchronously with inherited trace context or remove these two symbols
from the `@events` array and also remove/disable the corresponding span creation
logic referenced later in the module (the handlers that create spans for
subscribe_to_stream and delete_subscription found around the other telemetry
handling code). Update the `@events` constant to reflect the decision and ensure
the matching telemetry handler code (the functions creating spans for
subscribe_to_stream and delete_subscription) is cleaned up to avoid emitting
parentless spans.
---
Outside diff comments:
In `@lib/commanded/aggregates/aggregate_state_builder.ex`:
- Around line 193-210: The telemetry metadata docs are missing the new
caller_metadata field; update the telemetry event metadata specs for the load
and populate events (load :start, load :stop, populate :start, populate :stop)
to include caller_metadata so TelemetryRegistry exposes it; locate the
telemetry_metadata function and the telemetry_event/metadata specs for the
load/populate events and add caller_metadata to their maps/typed specs
consistently with how telemetry_metadata(%Aggregate{}) now returns it.
🪄 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: eeaa6e4f-f190-4d47-af13-38f2af3061ee
📒 Files selected for processing (12)
lib/commanded/aggregates/aggregate.exlib/commanded/aggregates/aggregate_state_builder.exlib/commanded/aggregates/supervisor.exlib/commanded/commands/dispatcher.exlib/commanded/opentelemetry/aggregate_populate.exlib/commanded/opentelemetry/commanded_attributes.exlib/commanded/opentelemetry/event_store.exlib/commanded/opentelemetry/event_store/adapters/event_store.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/event_store_test.exstest/support/opentelemetry_case.ex
🚧 Files skipped from review as they are similar to previous changes (6)
- test/opentelemetry/aggregate_test.exs
- test/opentelemetry/event_store_test.exs
- test/support/opentelemetry_case.ex
- lib/commanded/opentelemetry/event_store.ex
- lib/commanded/opentelemetry/commanded_attributes.ex
- lib/commanded/opentelemetry/helpers.ex
9754c14 to
e9bf396
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/commanded/opentelemetry/event_store.ex (1)
26-43:⚠️ Potential issue | 🟠 MajorGuard
setup/1against non-list/non-map arguments.Calling
config[:adapter]at line 41 will crash ifsetup/1receives an atom (e.g.,:enabled) ornil. The caller inlib/commanded/opentelemetry.ex:225–228only filters:disabled, passing any other value includingniltosetup/1. Normalize non-list inputs to an empty list to prevent runtime errors:Proposed fix
- def setup(config \\ []) do + def setup(config \\ []) + + def setup(config) when config in [nil, :enabled] do + setup([]) + end + + def setup(config) when is_list(config) or is_map(config) do🤖 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 26 - 43, The setup/1 function can crash when config is nil or an atom because it does config[:adapter]; before using it, normalize config to a keyword/map by assigning something like config = if is_map(config) or Keyword.keyword?(config), do: config, else: [] inside setup/1 so non-list/non-map inputs become an empty keyword list; then the later check if config[:adapter] == :enabled and call to Adapters.EventStore.setup() will be safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/commanded/opentelemetry/event_store.ex`:
- Around line 26-43: The setup/1 function can crash when config is nil or an
atom because it does config[:adapter]; before using it, normalize config to a
keyword/map by assigning something like config = if is_map(config) or
Keyword.keyword?(config), do: config, else: [] inside setup/1 so
non-list/non-map inputs become an empty keyword list; then the later check if
config[:adapter] == :enabled and call to Adapters.EventStore.setup() will be
safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14eb69ac-0126-4c18-97b7-346946f9aa5f
📒 Files selected for processing (11)
lib/commanded/opentelemetry.exlib/commanded/opentelemetry/commanded_attributes.exlib/commanded/opentelemetry/event_store.exlib/commanded/opentelemetry/event_store/adapters/event_store.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exstest/opentelemetry/event_handler_test.exstest/opentelemetry/event_store/adapters/event_store_test.exstest/opentelemetry/event_store_test.exstest/support/opentelemetry_case.ex
✅ Files skipped from review due to trivial changes (3)
- test/opentelemetry/aggregate_test.exs
- lib/commanded/opentelemetry/commanded_attributes.ex
- lib/commanded/opentelemetry/event_store/adapters/event_store.ex
🚧 Files skipped from review as they are similar to previous changes (4)
- test/support/opentelemetry_case.ex
- test/opentelemetry/event_handler_test.exs
- test/opentelemetry/application_test.exs
- lib/commanded/opentelemetry.ex
bd187ce to
189d043
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1 participant
Summary
straw-hat-team/eventstore) now emits its own[:eventstore, operation, suffix]telemetry events (ref: straw-hat-team/eventstore@57000ff). This hooks into those events directly so operators get span coverage at the storage layer, independent of commanded's own event store abstraction.OpenTelemetryCaseenvironment isolation so tests pass regardless of shell-levelOTEL_TRACES_EXPORTERconfiguration.