fix(telemetry): strip Elixir. prefix from commanded.application attribute - #93
Conversation
PR SummaryLow Risk Overview Adjusts span attribute assertions across aggregate, application dispatch, event handler, aggregate load/populate/snapshot, and event store tests to expect the new string values (and to use Reviewed by Cursor Bugbot for commit d102a65. 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 54 minutes and 23 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 (12)
WalkthroughThis PR normalizes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/commanded/opentelemetry/aggregate.ex (1)
50-56:⚠️ Potential issue | 🟡 MinorNormalize the adjacent consumer group application value too.
Line 56 fixes
commanded.applicationusingHelpers.module_name(meta.application), but line 50 still passes the rawmeta.applicationatom tomessaging_consumer_group_name(). When OpenTelemetry exporters stringify atoms, this will still surfaceElixir.MockAppinstead of justMockApp.Apply the same normalization:
Proposed fix
- {MessagingAttributes.messaging_consumer_group_name(), meta.application}, + {MessagingAttributes.messaging_consumer_group_name(), Helpers.module_name(meta.application)},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/opentelemetry/aggregate.ex` around lines 50 - 56, The messaging consumer group attribute currently uses the raw atom meta.application which yields "Elixir.MockApp" when exported; update the tuple {MessagingAttributes.messaging_consumer_group_name(), meta.application} to use the same normalization as commanded_application by calling Helpers.module_name(meta.application) so it becomes {MessagingAttributes.messaging_consumer_group_name(), Helpers.module_name(meta.application)}; this mirrors the change already made for CommandedAttributes.commanded_application and ensures consistent, non-Elixir-prefixed names in OpenTelemetry.lib/commanded/opentelemetry/event_handler.ex (1)
84-90:⚠️ Potential issue | 🟡 MinorNormalize consumer group names alongside
commanded.applicationin event and aggregate handlers.Lines 84/187 in
event_handler.exand line 50 inaggregate.exemit the raw application module formessaging.consumer.group.name, but the correspondingcommanded.applicationattributes normalize it withHelpers.module_name(). This leaves theElixir.prefix visible on event and aggregate handler spans.Apply the fix across all three locations:
Proposed fixes
lib/commanded/opentelemetry/event_handler.ex (line 84):
- {MessagingAttributes.messaging_consumer_group_name(), meta.application}, + {MessagingAttributes.messaging_consumer_group_name(), Helpers.module_name(meta.application)},lib/commanded/opentelemetry/event_handler.ex (line 187):
- {MessagingAttributes.messaging_consumer_group_name(), meta.application}, + {MessagingAttributes.messaging_consumer_group_name(), Helpers.module_name(meta.application)},lib/commanded/opentelemetry/aggregate.ex (line 50):
- {MessagingAttributes.messaging_consumer_group_name(), meta.application}, + {MessagingAttributes.messaging_consumer_group_name(), Helpers.module_name(meta.application)},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/opentelemetry/event_handler.ex` around lines 84 - 90, The Messaging consumer group attribute is emitting the raw application module (showing the Elixir. prefix); update all places that set {MessagingAttributes.messaging_consumer_group_name(), meta.application} to normalize the module name using Helpers.module_name(meta.application). Specifically, replace the tuple value for MessagingAttributes.messaging_consumer_group_name() in the event handler (both where attributes are assembled in handle spans and the other attribute block) and in the aggregate handler so they use Helpers.module_name(meta.application) instead of meta.application; leave the corresponding CommandedAttributes.commanded_application() usage unchanged.
🧹 Nitpick comments (1)
test/opentelemetry/application_test.exs (1)
101-101: LGTM — mixed literal/inspectstrategy is consistent with module-atom behavior.Using
inspect(meta.application)at lines 372/417 for dynamic cases and string literals elsewhere both produce the same output asHelpers.module_name/1for module atoms (e.g.,inspect(MockApp) == "MockApp"). This is safe here becausemeta.applicationis always a module atom in these factory-built metas.One small consideration: if
Helpers.module_name/1ever diverges frominspect/1for non-module inputs (e.g., handles nil or non-atoms specially), these tests would silently go out of sync with the implementation. For long-term robustness you could assert against a shared helper call rather than re-deriving withinspect/1, but it's optional given the factory guarantees.Also applies to: 229-229, 372-372, 417-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_test.exs` at line 101, Replace uses of inspect(meta.application) in the tests with a call to the shared helper Helpers.module_name/1 so the expectation follows the real implementation; locate the assertions that currently derive the application name via inspect(meta.application) and change them to assert Helpers.module_name(meta.application) (keeping the rest of the assertion text intact) to avoid future divergence between inspect/1 and Helpers.module_name/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 `@test/opentelemetry/event_handler_test.exs`:
- Line 109: Wrap the raw meta.application atoms with Helpers.module_name/1 when
building the messaging.consumer.group.name attribute so it emits the normalized
string (e.g., "MyApp.CommandedApp") instead of Elixir-prefixed atoms;
specifically update the attribute builders that set
"messaging.consumer.group.name" (see messaging_consumer_group_name / attribute
builder usage in EventHandler - functions where meta.application is used around
lines referenced and in Aggregate where meta.application is used) to call
Helpers.module_name(meta.application) before converting/assigning the attribute.
Ensure the same change is applied in both EventHandler and Aggregate attribute
builders to keep OTel name formatting consistent.
---
Outside diff comments:
In `@lib/commanded/opentelemetry/aggregate.ex`:
- Around line 50-56: The messaging consumer group attribute currently uses the
raw atom meta.application which yields "Elixir.MockApp" when exported; update
the tuple {MessagingAttributes.messaging_consumer_group_name(),
meta.application} to use the same normalization as commanded_application by
calling Helpers.module_name(meta.application) so it becomes
{MessagingAttributes.messaging_consumer_group_name(),
Helpers.module_name(meta.application)}; this mirrors the change already made for
CommandedAttributes.commanded_application and ensures consistent,
non-Elixir-prefixed names in OpenTelemetry.
In `@lib/commanded/opentelemetry/event_handler.ex`:
- Around line 84-90: The Messaging consumer group attribute is emitting the raw
application module (showing the Elixir. prefix); update all places that set
{MessagingAttributes.messaging_consumer_group_name(), meta.application} to
normalize the module name using Helpers.module_name(meta.application).
Specifically, replace the tuple value for
MessagingAttributes.messaging_consumer_group_name() in the event handler (both
where attributes are assembled in handle spans and the other attribute block)
and in the aggregate handler so they use Helpers.module_name(meta.application)
instead of meta.application; leave the corresponding
CommandedAttributes.commanded_application() usage unchanged.
---
Nitpick comments:
In `@test/opentelemetry/application_test.exs`:
- Line 101: Replace uses of inspect(meta.application) in the tests with a call
to the shared helper Helpers.module_name/1 so the expectation follows the real
implementation; locate the assertions that currently derive the application name
via inspect(meta.application) and change them to assert
Helpers.module_name(meta.application) (keeping the rest of the assertion text
intact) to avoid future divergence between inspect/1 and Helpers.module_name/1.
🪄 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: 8e75c79e-0762-485b-956b-962d25ca2d83
📒 Files selected for processing (12)
lib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/aggregate_populate.exlib/commanded/opentelemetry/aggregate_snapshot.exlib/commanded/opentelemetry/application.exlib/commanded/opentelemetry/event_handler.exlib/commanded/opentelemetry/event_store.extest/opentelemetry/aggregate_populate_test.exstest/opentelemetry/aggregate_snapshot_test.exstest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exstest/opentelemetry/event_handler_test.exstest/opentelemetry/event_store_test.exs
![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 3650d28. Configure here.
…bute - OTel convention requires module names without the BEAM-internal Elixir. prefix in attribute values Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
3650d28 to
d102a65
Compare
1 participant
Elixir.prefix in attribute values (e.g.Umbrella.CommandRouternotElixir.Umbrella.CommandRouter)commanded.applicationwas storing the raw module atom which OTel serializes viato_string/1, leaking theElixir.prefix into APM tools