fix(telemetry): add peer.service and DB semantic attributes to EventStore spans - #91
Conversation
PR SummaryMedium Risk Overview Adds helper logic to safely enrich spans from Reviewed by Cursor Bugbot for commit 8796a3f. 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 12 minutes and 27 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 (3)
WalkthroughThe OpenTelemetry instrumentation for EventStore is refactored to derive span metadata through adapter resolution. Previously, destination was computed directly from event metadata; now, the adapter is fetched and queried for connection configuration. The span kind changes from Changes
Sequence DiagramsequenceDiagram
actor Start as Telemetry Event
participant EST as EventStore Module
participant AR as Adapter Resolution
participant Config as Connection Config Lookup
participant SA as Span Attributes Builder
participant OT as OpenTelemetry Span
Start->>EST: telemetry :start event
EST->>AR: fetch_event_store_adapter(application)
AR->>AR: resolve adapter & meta
AR-->>EST: {adapter, adapter_meta}
EST->>Config: lookup_connection_config(adapter, event_store_name)
Config-->>EST: connection_config or []
EST->>SA: build attributes list
SA->>SA: maybe_add_db_system(adapter_type)
SA->>SA: maybe_add_connection_attributes(config)
SA-->>EST: enriched attributes
EST->>OT: create span(kind: :client, attributes)
OT-->>Start: span recorded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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 2 potential issues.
❌ 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 a4b7b85. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/opentelemetry/event_store_test.exs (2)
622-624: Nit: same whitespace issue as line 451Trailing spaces before the closing brace after removing the last comma.
Proposed diff
- "error.type": "Elixir.RuntimeError" } + "error.type": "Elixir.RuntimeError" + }🤖 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 622 - 624, The map literal contains trailing whitespace before the closing brace (after the "error.type": "Elixir.RuntimeError" entry); remove the extra spaces so the closing brace immediately follows the value (matching the style used at line 451). Locate the map in the test (the block containing "commanded.expected_version", "erlang.exception.kind", and "error.type") and delete the trailing spaces before the closing brace to keep consistent whitespace/formatting.
449-451: Nit: stray whitespace before closing braceWhen the last map entry's trailing comma was removed, leftover spaces/indentation were left before the
}. Same thing at line 624. A formatter pass will clean this up.Proposed diff
- "error.type": "Elixir.MatchError" } + "error.type": "Elixir.MatchError" + }🤖 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 449 - 451, The closing brace of the map literal that contains keys like "commanded.expected_version", "erlang.exception.kind" and "error.type" has stray whitespace/indentation before the `}` (also present at the other map near line 624); remove the extra spaces so the `}` directly follows the last entry (and run the Elixir formatter); locate the map in test/opentelemetry/event_store_test.exs (the failing map in the test assertions) and trim the indentation/whitespace before the closing brace to match standard formatting.
🤖 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/event_store.ex`:
- Around line 208-219: The db_system_for/1 function currently returns atoms
(:postgresql, :in_memory) which do not match OTel SemConv canonical string
values and use an unsupported "in_memory" token; change db_system_for/1 to
return the canonical string "postgresql" for
Commanded.EventStore.Adapters.EventStore and return nil (or skip setting
db.system) for the InMemory adapter; separately, avoid a hard compile‑time
dependency on EventStore.Config.lookup/1 in lookup_connection_config/2 by doing
a runtime-safe call (e.g., check Code.ensure_loaded?(EventStore.Config) and
function_exported?(EventStore.Config, :lookup, 1) before invoking, otherwise
return []) so you don’t emit Xref/compile warnings while preserving the current
rescue behavior.
In `@lib/commanded/opentelemetry/helpers.ex`:
- Around line 115-123: The helper maybe_add_connection_attributes currently sets
PeerAttributes.peer_service() to config[:database], which is wrong under
OpenTelemetry conventions and duplicates db.namespace; change the call so the
peer attribute is a service identifier (preferably service.peer.name for forward
compatibility) by accepting an extra parameter (e.g., event_store_name) or
letting the caller supply it; update maybe_add_connection_attributes to call
maybe_add_attr(PeerAttributes.peer_service() or the newer service.peer.name
attribute, event_store_name) instead of config[:database], and update the caller
in event_store.ex to pass event_store_name (e.g., inspect(event_store_name) or a
sensible default like "eventstore").
- Around line 115-126: The code calls the non-existent
EventStore.Config.lookup/1 which causes the rescue path to always return [] —
replace that call with EventStore.Config.parsed(event_store_name, :commanded)
(or EventStore.Config.get/2 as appropriate) where the lookup was used, and
update the input validation on maybe_add_connection_attributes to use an
explicit Keyword test (e.g., Keyword.keyword?(config)) instead of the pattern
guard [_ | _] = config so the function only attempts to add attributes when
config is a keyword list; keep the maybe_add_attr helper behavior intact.
---
Nitpick comments:
In `@test/opentelemetry/event_store_test.exs`:
- Around line 622-624: The map literal contains trailing whitespace before the
closing brace (after the "error.type": "Elixir.RuntimeError" entry); remove the
extra spaces so the closing brace immediately follows the value (matching the
style used at line 451). Locate the map in the test (the block containing
"commanded.expected_version", "erlang.exception.kind", and "error.type") and
delete the trailing spaces before the closing brace to keep consistent
whitespace/formatting.
- Around line 449-451: The closing brace of the map literal that contains keys
like "commanded.expected_version", "erlang.exception.kind" and "error.type" has
stray whitespace/indentation before the `}` (also present at the other map near
line 624); remove the extra spaces so the `}` directly follows the last entry
(and run the Elixir formatter); locate the map in
test/opentelemetry/event_store_test.exs (the failing map in the test assertions)
and trim the indentation/whitespace before the closing brace to match standard
formatting.
🪄 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: 58523421-cfde-4832-8f56-a9a7980fdf17
📒 Files selected for processing (3)
lib/commanded/opentelemetry/event_store.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/event_store_test.exs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1 participant
Summary
kind: :internaland lack database connection attributes — everything is lumped under the parent BEAM node'sservice.name:clientand addingdb.system,server.address,server.port,db.namespace, andpeer.servicefrom the EventStore's runtime connection config enables APM service map discoveryEventStore.Config.lookup/1with a rescue fallback for adapters that don't use it (e.g. InMemory in tests)