feat(telemetry): keep returned command errors out of failed spans - #82
Conversation
PR SummaryMedium Risk Overview Wires the validated config through Reviewed by Cursor Bugbot for commit b9e170d. 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 43 minutes and 34 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 (7)
WalkthroughThis PR introduces configurable error status handling for OpenTelemetry spans in Commanded. It adds an Changes
Sequence DiagramsequenceDiagram
participant Code as Commanded Code
participant Telemetry as Telemetry System
participant Handler as Aggregate/Application Handler
participant Helper as Helpers.set_error_status
participant Callback as error_status Callback
participant Span as OpenTelemetry Span
Code->>Telemetry: Emit :stop event with error metadata
Telemetry->>Handler: Invoke handler (measurements, meta, config)
Handler->>Helper: set_error_status(ctx, error, event_name, measurements, meta, config, tracer_id)
Helper->>Helper: Check config[:error_status]
alt Callback configured
Helper->>Callback: fun(event_name, measurements, meta, config)
Callback-->>Helper: Return status code (:error, :unset, :ok, or nil)
Helper->>Helper: apply_error_status(status_code)
else No callback
Helper->>Helper: Use default :error status
end
Helper->>Span: Set span status (with error description or unset)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 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 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 d445522. 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: 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/opentelemetry/helpers.ex`:
- Around line 72-75: The current case in helpers.ex calls the configured
:error_status callback directly (the branch with fun -> fun.(meta, config)),
which can raise and skip the fallback mapping; change this to invoke the
callback inside a try/rescue (and optionally catch) block so any exception falls
back to :error, emit or dispatch a warning telemetry event when the callback
fails, and keep the existing nil -> :error behavior intact; update the branch
handling for Keyword.get(config, :error_status) to catch errors from fun.(meta,
config), log/telemetry the failure, and return :error on exception.
🪄 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: 9c0984e9-c40d-46e7-95ec-8b027888c983
📒 Files selected for processing (5)
guides/howtos/setting-up-opentelemetry-tracing.mdlib/commanded/opentelemetry.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exs
🚧 Files skipped from review as they are similar to previous changes (4)
- test/opentelemetry/application_test.exs
- guides/howtos/setting-up-opentelemetry-tracing.md
- test/opentelemetry/aggregate_test.exs
- lib/commanded/opentelemetry.ex
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/commanded/opentelemetry/helpers.ex (1)
70-75:⚠️ Potential issue | 🟠 MajorGuard
error_statuscallback execution to preserve fallback behavior.The direct call on Line 74 (
fun.(...)) can raise/throw and bypassapply_error_status/4, making the error path brittle. Wrap it withtry/rescue/catch, emit warning telemetry, and fallback to:error.Suggested patch
def set_error_status(ctx, error, event_name, measurements, meta, config, tracer_id) do status_code = case Keyword.get(config, :error_status) do nil -> :error - fun when is_function(fun, 4) -> fun.(event_name, measurements, meta, config) + fun when is_function(fun, 4) -> + try do + fun.(event_name, measurements, meta, config) + rescue + exception -> + :telemetry.execute( + [:commanded, :opentelemetry, :warning], + %{count: 1}, + %{ + message: "error_status callback raised, falling back to :error", + error: exception, + tracer_id: tracer_id + } + ) + + :error + catch + kind, reason -> + :telemetry.execute( + [:commanded, :opentelemetry, :warning], + %{count: 1}, + %{ + message: "error_status callback threw, falling back to :error", + error: {kind, reason}, + tracer_id: tracer_id + } + ) + + :error + end end apply_error_status(ctx, status_code, error, tracer_id) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/opentelemetry/helpers.ex` around lines 70 - 75, In set_error_status, guard the dynamic call to the configured :error_status callback so it cannot raise/throw and bypass the normal fallback: wrap the call to the function retrieved from Keyword.get(config, :error_status) in a try/rescue/catch block (around the branch where fun when is_function(fun, 4) -> fun.(...)), and on any exception/throw log/emit a warning telemetry event (including event_name, measurements, meta and the error) and return the fallback :error; ensure that apply_error_status/4 remains the intended safe alternative path when the callback is absent or fails so callers still receive :error on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/commanded/opentelemetry/helpers.ex`:
- Around line 70-75: In set_error_status, guard the dynamic call to the
configured :error_status callback so it cannot raise/throw and bypass the normal
fallback: wrap the call to the function retrieved from Keyword.get(config,
:error_status) in a try/rescue/catch block (around the branch where fun when
is_function(fun, 4) -> fun.(...)), and on any exception/throw log/emit a warning
telemetry event (including event_name, measurements, meta and the error) and
return the fallback :error; ensure that apply_error_status/4 remains the
intended safe alternative path when the callback is absent or fails so callers
still receive :error on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb288273-a022-47b1-be45-bcf66957f4e1
📒 Files selected for processing (7)
guides/howtos/setting-up-opentelemetry-tracing.mdlib/commanded/opentelemetry.exlib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/application.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/aggregate_test.exstest/opentelemetry/application_test.exs
✅ Files skipped from review due to trivial changes (1)
- guides/howtos/setting-up-opentelemetry-tracing.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/opentelemetry/aggregate_test.exs
- test/opentelemetry/application_test.exs
- lib/commanded/opentelemetry/application.ex
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
2745d15 to
b9e170d
Compare
1 participant
:stoperrors.