feat: Add OpenTelemetry instrumentation for event handlers - #41
Conversation
PR SummaryIntroduces first‑class OpenTelemetry tracing for Commanded event processing.
Written by Cursor Bugbot for commit a3418c6. This will update automatically on new commits. Configure here. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces comprehensive OpenTelemetry tracing integration to Commanded, adding span creation and context propagation for event handlers and batch processing. It includes new core modules, middleware updates, configuration changes, documentation guides, and extensive test infrastructure supporting the tracing feature. Changes
Sequence DiagramsequenceDiagram
participant App as Application Startup
participant OT as Commanded.OpenTelemetry
participant Handler as Event Handler
participant Telemetry as Telemetry System
participant Span as OpenTelemetry Span
participant Backend as Tracing Backend
App->>OT: setup(span_relationship: :child)
OT->>Handler: setup(opts)
Handler->>Telemetry: attach event listeners
Telemetry-->>Handler: ready
Handler->>Telemetry: [:commanded, :event, :start]
Telemetry->>Span: create span
Span->>Span: add attributes (handler, event, correlation)
Span->>Backend: export span context
Handler->>Span: process event
alt Success
Span->>Span: set status ok
else Error
Span->>Span: set status error with message
end
Handler->>Telemetry: [:commanded, :event, :stop]
Telemetry->>Span: end span
Span->>Backend: record span
Handler->>Telemetry: [:commanded, :event, :batch_start]
Telemetry->>Span: create batch span
Span->>Span: add batch attributes
Handler->>Span: process batch events
Handler->>Telemetry: [:commanded, :event, :batch_stop]
Telemetry->>Span: end batch span
Span->>Backend: record batch span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
2f6c832 to
af578fb
Compare
3ca5650 to
97028f5
Compare
97028f5 to
8126b0e
Compare
a62a126 to
07798dd
Compare
8105748 to
0d2427f
Compare
a6f562c to
e411579
Compare
f80a1dd to
0c177d6
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
0c177d6 to
a3418c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/opentelemetry/event_handler_test.exs`:
- Around line 62-65: Replace the brittle assertion that checks the total number
of handlers (length(handlers) == 1) with an assertion that specifically looks
for your handler id: call :telemetry.list_handlers([:commanded, :event, :handle,
:start]), then filter or find handlers whose id equals the exact id used earlier
in the test (the same pattern used at lines 49-51), e.g. Enum.filter(handlers,
fn h -> handler_id_match?(h, expected_id) end) or Enum.any?(handlers,
&match_expected_id?/1), and assert that the filtered result contains your
handler (length == 1 or assert true for Enum.any?). Ensure you reference the
exact expected handler id constant/name used earlier in the file.
- Around line 921-933: The detach_handlers/0 helper is detaching all telemetry
handlers for the given events; change it to only detach handlers installed by
Commanded.OpenTelemetry.EventHandler by checking each handler.id from
:telemetry.list_handlers(event) before calling :telemetry.detach. Inside
detach_handlers/0 (and the inner loop over for handler <-
:telemetry.list_handlers(event)), only call :telemetry.detach(handler.id) when
handler.id identifies the Commanded.OpenTelemetry.EventHandler (e.g., matches
the module atom or contains Commanded.OpenTelemetry.EventHandler in the id
tuple) and skip detaching any other handlers.
♻️ Duplicate comments (1)
lib/commanded/opentelemetry/event_handler.ex (1)
12-46: Propagateattach_manyerrors instead of raising.The current
:ok = ...pattern will raise on{:error, :already_exists}instead of returning that tuple. Consider chaining the results so the caller can handle the error cleanly.✅ Suggested fix
def setup(opts \\ []) do span_relationship = Keyword.get(opts, :span_relationship, :link) config = %{span_relationship: span_relationship} - :ok = attach_handle_handlers(config) - :ok = attach_batch_handlers(config) - - :ok + with :ok <- attach_handle_handlers(config), + :ok <- attach_batch_handlers(config) do + :ok + end end
🧹 Nitpick comments (1)
test/support/factory.ex (1)
278-318: Allow measurement overrides inbuild_telemetry_event.Right now
optsonly affect metadata; passing them through lets tests overridesystem_time/durationwhen needed.♻️ Suggested update
def build_telemetry_event(:start, opts) do - measurements = build_telemetry_start_measurements() + measurements = build_telemetry_start_measurements(opts) metadata = build_event_handler_metadata(opts) event_name = [:commanded, :event, :handle, :start] {event_name, measurements, metadata} end def build_telemetry_event(:stop, opts) do - measurements = build_telemetry_stop_measurements() + measurements = build_telemetry_stop_measurements(opts) metadata = build_event_handler_metadata(opts) event_name = [:commanded, :event, :handle, :stop] {event_name, measurements, metadata} end def build_telemetry_event(:exception, opts) do - measurements = build_telemetry_exception_measurements() + measurements = build_telemetry_exception_measurements(opts) metadata = build_exception_metadata(opts) event_name = [:commanded, :event, :handle, :exception] {event_name, measurements, metadata} end def build_telemetry_event(:batch_start, opts) do - measurements = build_telemetry_start_measurements() + measurements = build_telemetry_start_measurements(opts) metadata = build_batch_handler_metadata(opts) event_name = [:commanded, :event, :batch, :start] {event_name, measurements, metadata} end def build_telemetry_event(:batch_stop, opts) do - measurements = build_telemetry_stop_measurements() + measurements = build_telemetry_stop_measurements(opts) metadata = build_batch_handler_metadata(opts) event_name = [:commanded, :event, :batch, :stop] {event_name, measurements, metadata} end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
guides/explanations/built-in-vs-external-projections.mdguides/explanations/fork-differences.mdguides/howtos/setting-up-opentelemetry-tracing.mdlib/commanded/middleware/trace_context_propagator.exlib/commanded/opentelemetry.exlib/commanded/opentelemetry/commanded_attributes.exlib/commanded/opentelemetry/event_handler.exmix.exstest/middleware/trace_context_propagator_test.exstest/opentelemetry/event_handler_test.exstest/support/factory.extest/support/opentelemetry_case.extest/support/test_domain.ex
💤 Files with no reviewable changes (1)
- guides/explanations/built-in-vs-external-projections.md
🧰 Additional context used
🧬 Code graph analysis (3)
lib/commanded/opentelemetry.ex (1)
lib/commanded/opentelemetry/event_handler.ex (1)
setup(12-20)
lib/commanded/opentelemetry/event_handler.ex (1)
lib/commanded/opentelemetry.ex (1)
setup(94-101)
test/support/factory.ex (1)
test/opentelemetry/event_handler_test.exs (1)
build_recorded_event(880-887)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Quality Assurance (1.19.x, 27)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (23)
lib/commanded/middleware/trace_context_propagator.ex (2)
44-61: Clear pre-dispatch trace context docs.The updated docstring clarifies behavior and improves readability without changing logic.
69-75: Marking post-dispatch hooks as internal is fine.The
@docfalse +@implannotations keep the public surface tidy.test/support/test_domain.ex (1)
1-40: Clean, focused test fixtures.Structs are simple and appropriately scoped for test support.
mix.exs (3)
186-208: Docs grouping for OpenTelemetry modules is consistent.Module grouping and nesting aligns with the new API surface.
249-261: Dialyzer PLT additions are appropriate.Including OTel apps in PLT aligns with new dependencies.
70-79: OpenTelemetry and NimbleOptions versions are compatible with the project's Elixir ~> 1.12 requirement.All selected versions check out:
- nimble_options 1.1.1 (latest) has no minimum Elixir constraint
- opentelemetry_api 1.0+ requires Elixir 1.11+, satisfied by 1.12
- opentelemetry_semantic_conventions 1.27 requires Elixir ~> 1.12, which matches the project exactly
- OTP 22+ (required by opentelemetry_api) is supported by Elixir 1.12
No compatibility concerns.
lib/commanded/opentelemetry.ex (1)
1-101: API surface and docs are clear.Setup flow and span relationship semantics are well documented and easy to follow.
guides/howtos/setting-up-opentelemetry-tracing.md (1)
1-62: Docs are clear and actionable.The guide gives concise setup steps and configuration examples.
test/support/factory.ex (5)
6-114: LGTM — command/event builder helpers look consistent.
116-152: LGTM — RecordedEvent factory is well-structured.
154-188: LGTM — telemetry measurement builders are straightforward.
190-276: LGTM — metadata builders are cohesive.
320-388: LGTM — scenario builders read well.lib/commanded/opentelemetry/commanded_attributes.ex (1)
1-144: LGTM — clear, centralized attribute constants.lib/commanded/opentelemetry/event_handler.ex (7)
48-115: LGTM — handle start span setup looks solid.
117-149: LGTM — stop/exception span handling is clear.
151-195: LGTM — batch start path looks good.
197-229: LGTM — batch stop/exception handling looks good.
231-253: LGTM — error helpers are tidy.
254-299: LGTM — context helpers are concise.
301-307: LGTM — small helpers are fine.guides/explanations/fork-differences.md (1)
220-245: LGTM — OpenTelemetry feature entry is clear.test/support/opentelemetry_case.ex (1)
1-54: LGTM — test case template setup/cleanup reads cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Yordis Prieto yordis.prieto@gmail.com