feat(telemetry): add registry adapter to aggregate execute span - #71
Conversation
PR SummaryLow Risk Overview Updates Reviewed by Cursor Bugbot for commit 2ca8906. 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 8 minutes and 2 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 (5)
WalkthroughAggregate telemetry now includes the configured registry adapter in its metadata; the OpenTelemetry instrumentation exposes that value as a new span attribute used during aggregate execute spans. Tests and factory helpers were updated to provide the new metadata field. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/aggregates/aggregate.ex`:
- Around line 772-782: The docs for the "execute" telemetry event metadata are
out of sync with runtime: add the registry_adapter field to the documented
metadata contract so it matches the emitted payload (the runtime map includes
registry_adapter from Commanded.Application.registry_adapter). Update the
execute telemetry metadata definitions (the doc/typedoc or `@doc` block in
aggregate.ex that lists metadata keys for the execute event) to include
registry_adapter and describe its value/type consistently with the other fields.
🪄 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: 17ac352d-aa7b-4404-b731-5640e5383d05
📒 Files selected for processing (3)
lib/commanded/aggregates/aggregate.exlib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/commanded_attributes.ex
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/opentelemetry/aggregate_test.exs (1)
116-116: Add one non-default adapter test case to prevent hardcoded-value regressions.Right now all new assertions validate only
Commanded.Registration.LocalRegistry. A single case using a different adapter value would prove this is propagated from metadata/config, not constantized.Example test addition
+ test "propagates non-default registry adapter into span attributes" do + aggregate_uuid = UUID.uuid4() + causation_id = UUID.uuid4() + correlation_id = UUID.uuid4() + + meta = + Factory.build_aggregate_execute_metadata( + aggregate_uuid: aggregate_uuid, + causation_id: causation_id, + correlation_id: correlation_id, + registry_adapter: Commanded.Registration.GlobalRegistry + ) + + :telemetry.span([:commanded, :aggregate, :execute], meta, fn -> + {:ok, meta} + end) + + assert_receive {:span, span(attributes: attributes)}, 1000 + attrs = :otel_attributes.map(attributes) + assert attrs[:"commanded.registry.adapter"] == "Commanded.Registration.GlobalRegistry" + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/aggregate_test.exs` at line 116, Add a test in aggregate_test.exs that uses a non-default adapter value for the "commanded.registry.adapter" metadata key (e.g., "Some.Custom.Adapter") to ensure the adapter value is propagated from metadata/config and not hardcoded; create/setup the event or request with that metadata value, run the same aggregation/assertion logic used in the existing tests, and assert the aggregated result contains the non-default "commanded.registry.adapter" string rather than only accepting "Commanded.Registration.LocalRegistry".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/opentelemetry/aggregate_test.exs`:
- Line 116: Add a test in aggregate_test.exs that uses a non-default adapter
value for the "commanded.registry.adapter" metadata key (e.g.,
"Some.Custom.Adapter") to ensure the adapter value is propagated from
metadata/config and not hardcoded; create/setup the event or request with that
metadata value, run the same aggregation/assertion logic used in the existing
tests, and assert the aggregated result contains the non-default
"commanded.registry.adapter" string rather than only accepting
"Commanded.Registration.LocalRegistry".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66b1bca2-58e8-4a86-8972-9ff03a05ed05
📒 Files selected for processing (2)
test/opentelemetry/aggregate_test.exstest/support/factory.ex
4b29b2d to
7e22292
Compare
Signed-off-by: Yordis Prieto <yordis@example.com> Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com> test(telemetry): cover commanded.registry.adapter attribute in aggregate execute span Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com> docs: temp Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
7e22292 to
2ca8906
Compare
:globalregistry, the registry adapter introduces per-call distributed lookup latency that is otherwise invisible in traces — surfacing the adapter on everyexecutespan makes it possible to filter and compare latency by registry type in observability tooling