Repository navigation
fix: preserve active OTel context in application dispatch - #54
Merged
Merged
Conversation
PR SummaryMedium Risk Overview Refactors propagation utilities by replacing Written by Cursor Bugbot for commit 87d0611. This will update automatically on new commits. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces unconditional metadata-based OTEL context attachment with an explicit extract/clear/attach flow: helpers now expose Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant HTTP as "HTTP/span (root)"
participant Router as "Router / Middleware"
participant Dispatcher as "Dispatch span"
participant Aggregate as "Aggregate span"
participant Handler as "EventHandler span"
rect rgba(200,200,255,0.5)
Client->>HTTP: incoming request (creates HTTP span)
end
rect rgba(200,255,200,0.5)
HTTP->>Router: invoke command (middleware injects traceparent into metadata)
Router->>Dispatcher: start dispatch (child of HTTP if active)
end
rect rgba(255,230,200,0.5)
Dispatcher->>Aggregate: call aggregate (metadata passed)
Aggregate->>Aggregate: call Helpers.extract_propagated_ctx(metadata)
Aggregate->>Aggregate: attach extracted ctx OR call Helpers.clear_ctx()
Aggregate->>Handler: persist event (handler receives metadata)
end
rect rgba(255,200,200,0.5)
Handler->>Handler: attach propagated ctx (child) OR clear and start new trace + add link back
Handler-->>Client: finish
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/release-please.yml (1)
21-21: Pin GitHub Action to an immutable commit SHA.Line 21 uses a floating major tag (
@v4), which can change over time. Prefer SHA pinning for reproducibility and supply-chain safety.Proposed change
- uses: googleapis/release-please-action@v4 + uses: googleapis/release-please-action@c3fc4de07084f75a2b61a5b933069bda6edf3d5c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-please.yml at line 21, Replace the floating tag "uses: googleapis/release-please-action@v4" with an immutable commit SHA for the googleapis/release-please-action action: locate the latest stable commit SHA in the action's repository (or the commit you want to pin), and update the workflow entry to use that SHA instead of "@v4" (e.g., "uses: googleapis/release-please-action@<commit-sha>"). Ensure the SHA you pick is documented in the repo or PR so future maintainers know why it was pinned and consider regularly updating the pinned SHA via CI or an automated dependabot-style workflow.test/opentelemetry/application_test.exs (2)
439-450: Consider extractingencode_traceparent/1into shared test support.This helper duplicates logic already present in
test/opentelemetry/aggregate_test.exs(Lines 599-609). Moving it to shared test support would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_test.exs` around lines 439 - 450, The helper encode_traceparent/1 is duplicated across tests; extract it into a shared test support module (e.g., an OpentelemetryTestHelpers module) and replace the local definitions in application_test.exs and aggregate_test.exs with calls/imports to that helper; update test/support to define encode_traceparent/1 (mirroring the existing implementation) and add the support module to ExUnit configuration so both tests can call encode_traceparent/1 without duplication.
155-175: Strengthen the precedence test by using a conflictingtraceparent.Right now the metadata
traceparentis derived from the same active span context, so this can still pass if metadata extraction incorrectly wins. Use a different/foreign traceparent and assert the dispatch span still chooses the active parent.Suggested test hardening diff
- traceparent = encode_traceparent(ctx) + {foreign_trace_id, foreign_span_id, traceparent} = + Tracer.with_span "foreign.request" do + foreign_ctx = Tracer.current_span_ctx() + { + :otel_span.trace_id(foreign_ctx), + :otel_span.span_id(foreign_ctx), + encode_traceparent(foreign_ctx) + } + end @@ - {parent_trace_id, parent_span_id} + {parent_trace_id, parent_span_id, foreign_trace_id, foreign_span_id} end @@ - assert dispatch_parent_span_id == parent_span_id, + assert dispatch_parent_span_id == parent_span_id, "Dispatch span should be a child of the active span, not the extracted traceparent." + + refute dispatch_trace_id == foreign_trace_id + refute dispatch_parent_span_id == foreign_span_idAlso applies to: 191-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_test.exs` around lines 155 - 175, The test "dispatch span becomes a child even when traceparent is in metadata" currently injects a traceparent derived from the same active span; change it to inject a conflicting/foreign traceparent so we can assert the active process span still wins. Specifically, when using Tracer.with_span and encode_traceparent to build metadata via Factory.build_application_dispatch_metadata, replace the encoded traceparent with one generated from a different/new span or a manually constructed traceparent with a different trace_id/span_id, then assert the dispatch span chooses the Tracer.with_span context (active parent). Apply the same change to the similar test around lines 191-196 to harden that assertion as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release-please.yml:
- Line 21: Replace the floating tag "uses: googleapis/release-please-action@v4"
with an immutable commit SHA for the googleapis/release-please-action action:
locate the latest stable commit SHA in the action's repository (or the commit
you want to pin), and update the workflow entry to use that SHA instead of "@v4"
(e.g., "uses: googleapis/release-please-action@<commit-sha>"). Ensure the SHA
you pick is documented in the repo or PR so future maintainers know why it was
pinned and consider regularly updating the pinned SHA via CI or an automated
dependabot-style workflow.
In `@test/opentelemetry/application_test.exs`:
- Around line 439-450: The helper encode_traceparent/1 is duplicated across
tests; extract it into a shared test support module (e.g., an
OpentelemetryTestHelpers module) and replace the local definitions in
application_test.exs and aggregate_test.exs with calls/imports to that helper;
update test/support to define encode_traceparent/1 (mirroring the existing
implementation) and add the support module to ExUnit configuration so both tests
can call encode_traceparent/1 without duplication.
- Around line 155-175: The test "dispatch span becomes a child even when
traceparent is in metadata" currently injects a traceparent derived from the
same active span; change it to inject a conflicting/foreign traceparent so we
can assert the active process span still wins. Specifically, when using
Tracer.with_span and encode_traceparent to build metadata via
Factory.build_application_dispatch_metadata, replace the encoded traceparent
with one generated from a different/new span or a manually constructed
traceparent with a different trace_id/span_id, then assert the dispatch span
chooses the Tracer.with_span context (active parent). Apply the same change to
the similar test around lines 191-196 to harden that assertion as well.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release-please.ymllib/commanded/opentelemetry/application.extest/opentelemetry/application_test.exs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/opentelemetry/application_test.exs (1)
439-449: Consider extractingencode_traceparent/1to a shared test helper.This function is duplicated in
test/opentelemetry/aggregate_test.exs(lines 599-609). Consider moving it to a shared test support module to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_test.exs` around lines 439 - 449, The encode_traceparent/1 helper is duplicated across test modules; extract it into a single shared test support module (e.g., create a module like OpentelemetryTestHelpers with a public encode_traceparent/1 function), move the implementation there (preserving the :otel_span.trace_id/1, :otel_span.span_id/1 and trace_flags logic), update the test modules to call or import OpentelemetryTestHelpers.encode_traceparent/1 instead of defining it locally, and ensure the support module is loaded for tests (require importing or adding it via test_helper setup so both test suites use the shared helper).
🤖 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/application_test.exs`:
- Around line 155-196: The test encodes the traceparent from the same context
(ctx) used to extract parent_trace_id and parent_span_id, so both the active
span and traceparent metadata contain identical IDs. This means the assertions
will pass regardless of whether the dispatch span actually uses the active span
or the extracted traceparent. To properly verify that the active span is
preferred, generate a traceparent from a different trace context (not from ctx)
and pass that as the traceparent in the metadata. This way, if the
implementation incorrectly uses the extracted traceparent instead of the active
span, the assertions will fail as intended.
---
Nitpick comments:
In `@test/opentelemetry/application_test.exs`:
- Around line 439-449: The encode_traceparent/1 helper is duplicated across test
modules; extract it into a single shared test support module (e.g., create a
module like OpentelemetryTestHelpers with a public encode_traceparent/1
function), move the implementation there (preserving the :otel_span.trace_id/1,
:otel_span.span_id/1 and trace_flags logic), update the test modules to call or
import OpentelemetryTestHelpers.encode_traceparent/1 instead of defining it
locally, and ensure the support module is loaded for tests (require importing or
adding it via test_helper setup so both test suites use the shared helper).
yordis
force-pushed
the
fix-otel-1
branch
2 times, most recently
from
February 26, 2026 05:29
cbdf3da to
e1a325b
Compare
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 `@test/opentelemetry/helpers_test.exs`:
- Around line 172-181: The test calls an undefined function span_ctx/2 to get
trace_flags; replace that call with the proper OpenTelemetry accessor
:otel_span.trace_flags(span_ctx) to retrieve trace flags, updating
encode_traceparent/1 in test/opentelemetry/helpers_test.exs (function
encode_traceparent), and the identical occurrences in
test/opentelemetry/event_handler_test.exs and
test/opentelemetry/aggregate_test.exs so they all use
:otel_span.trace_flags(span_ctx) instead of span_ctx(span_ctx, :trace_flags).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/application.exlib/commanded/opentelemetry/event_handler.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/application_e2e_test.exstest/opentelemetry/application_test.exstest/opentelemetry/helpers_test.exs
💤 Files with no reviewable changes (1)
- lib/commanded/opentelemetry/application.ex
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/opentelemetry/application_e2e_test.exs (2)
163-168: The 1-second timeout incollect_all_spansmay cause slow test execution.This helper waits the full timeout before returning, which adds 1 second to test duration. Combined with the
Process.sleep(200), test execution is artificially extended. Consider using a shorter timeout or implementing early exit when no more spans are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_e2e_test.exs` around lines 163 - 168, The helper collect_all_spans currently blocks for 1_000ms on the receive after clause which adds unnecessary delay to tests; change its behavior by reducing the after timeout to a much smaller value (e.g., 50-100ms) or implement an early-exit strategy: accept an expected span count parameter (or poll until no message seen for a short backoff) and return as soon as that count is collected, updating calls to collect_all_spans accordingly (reference the collect_all_spans/1 function and any test calls that rely on it, and consider also reducing or removing the separate Process.sleep(200) where tests wait).
79-79: Consider replacingProcess.sleepwith a deterministic wait mechanism.Using
Process.sleep(200)makes this test potentially flaky, especially under varying system loads. While I understand it's waiting for async span emission, consider using a more deterministic approach like polling with a bounded retry or using a test helper that waits for specific spans to appear.💡 Example approach using a polling helper
- Process.sleep(200) - - spans = collect_all_spans() + spans = await_spans(expected_count: 4, timeout: 2000)Where
await_spanspolls until the expected number of spans is received or timeout is reached:defp await_spans(opts) do expected = Keyword.get(opts, :expected_count, 1) timeout = Keyword.get(opts, :timeout, 2000) await_spans([], expected, System.monotonic_time(:millisecond) + timeout) end defp await_spans(acc, expected, deadline) when length(acc) >= expected, do: Enum.reverse(acc) defp await_spans(acc, expected, deadline) do remaining = deadline - System.monotonic_time(:millisecond) if remaining <= 0 do Enum.reverse(acc) else receive do {:span, s} -> await_spans([s | acc], expected, deadline) after min(100, remaining) -> await_spans(acc, expected, deadline) end end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/application_e2e_test.exs` at line 79, Replace the flaky Process.sleep(200) call with a deterministic polling helper: remove Process.sleep/1 and call a helper like await_spans(expected_count: 1, timeout: 2000) (or await_spans() with defaults) that polls until the expected spans arrive; implement await_spans/1 plus internal clauses await_spans(acc, expected, deadline) and a receive/after loop (as in the example) so the test waits deterministically for {:span, s} messages instead of sleeping.
🤖 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/application_e2e_test.exs`:
- Around line 98-108: The assertion block around span(handle_span, :trace_id),
span(handle_span, :parent_span_id), :otel_links.list(span(handle_span, :links)),
and the link(...) assertions is misformatted and failing CI; reformat that
entire block (either run mix format or adjust spacing/line breaks) so it matches
Elixir formatter expectations—ensure the pipeline/assert indentation and string
concatenation for the last assert remain valid and that lines with
span(handle_span, :parent_span_id) and the link(...) assertions are properly
indented and line-broken consistent with mix format.
---
Nitpick comments:
In `@test/opentelemetry/application_e2e_test.exs`:
- Around line 163-168: The helper collect_all_spans currently blocks for 1_000ms
on the receive after clause which adds unnecessary delay to tests; change its
behavior by reducing the after timeout to a much smaller value (e.g., 50-100ms)
or implement an early-exit strategy: accept an expected span count parameter (or
poll until no message seen for a short backoff) and return as soon as that count
is collected, updating calls to collect_all_spans accordingly (reference the
collect_all_spans/1 function and any test calls that rely on it, and consider
also reducing or removing the separate Process.sleep(200) where tests wait).
- Line 79: Replace the flaky Process.sleep(200) call with a deterministic
polling helper: remove Process.sleep/1 and call a helper like
await_spans(expected_count: 1, timeout: 2000) (or await_spans() with defaults)
that polls until the expected spans arrive; implement await_spans/1 plus
internal clauses await_spans(acc, expected, deadline) and a receive/after loop
(as in the example) so the test waits deterministically for {:span, s} messages
instead of sleeping.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/application.exlib/commanded/opentelemetry/event_handler.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/application_e2e_test.exstest/opentelemetry/application_test.exstest/opentelemetry/helpers_test.exs
💤 Files with no reviewable changes (1)
- lib/commanded/opentelemetry/application.ex
🚧 Files skipped from review as they are similar to previous changes (3)
- test/opentelemetry/application_test.exs
- test/opentelemetry/helpers_test.exs
- lib/commanded/opentelemetry/aggregate.ex
yordis
force-pushed
the
fix-otel-1
branch
2 times, most recently
from
February 26, 2026 05:56
beff3b1 to
d816bf9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/opentelemetry/helpers_test.exs (1)
172-182:⚠️ Potential issue | 🔴 Critical
span_ctx/2is undefined — this will fail to compile.Line 175 calls
span_ctx(span_ctx, :trace_flags)but no such function exists. The OpenTelemetry Erlang API doesn't provide a direct accessor fortrace_flags.Proposed fix — use hardcoded flag for sampled traces
defp encode_traceparent(span_ctx) do trace_id = :otel_span.trace_id(span_ctx) span_id = :otel_span.span_id(span_ctx) - trace_flags = span_ctx(span_ctx, :trace_flags) + # Sampled flag = 1 (spans created via Tracer.with_span are sampled by default in tests) + trace_flags = 1 hex_trace_id = :io_lib.format("~32.16.0b", [trace_id]) |> IO.iodata_to_binary() hex_span_id = :io_lib.format("~16.16.0b", [span_id]) |> IO.iodata_to_binary() hex_flags = :io_lib.format("~2.16.0b", [trace_flags]) |> IO.iodata_to_binary() "00-#{hex_trace_id}-#{hex_span_id}-#{hex_flags}" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opentelemetry/helpers_test.exs` around lines 172 - 182, The call to span_ctx(span_ctx, :trace_flags) in encode_traceparent is invalid; instead set trace_flags to a known value (e.g., 1 for sampled) or obtain it via a supported API; update encode_traceparent to compute trace_flags as an integer (use 1 for sampled traces) and keep the rest of the hex formatting logic using trace_id = :otel_span.trace_id(span_ctx) and span_id = :otel_span.span_id(span_ctx) so hex_flags uses that integer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/opentelemetry/helpers_test.exs`:
- Around line 172-182: The call to span_ctx(span_ctx, :trace_flags) in
encode_traceparent is invalid; instead set trace_flags to a known value (e.g., 1
for sampled) or obtain it via a supported API; update encode_traceparent to
compute trace_flags as an integer (use 1 for sampled traces) and keep the rest
of the hex formatting logic using trace_id = :otel_span.trace_id(span_ctx) and
span_id = :otel_span.span_id(span_ctx) so hex_flags uses that integer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/commanded/opentelemetry/aggregate.exlib/commanded/opentelemetry/application.exlib/commanded/opentelemetry/event_handler.exlib/commanded/opentelemetry/helpers.extest/opentelemetry/application_e2e_test.exstest/opentelemetry/application_test.exstest/opentelemetry/helpers_test.exs
💤 Files with no reviewable changes (1)
- lib/commanded/opentelemetry/application.ex
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Helpers.attach_ctx/1unconditionally was destroying the active parent span context (e.g., gRPC/HTTP handler span), replacing it with either extracted traceparent headers or an empty context.maybe_attach_ctx/1which checks for a valid span via:otel_span.is_valid/1before deciding whether to extract from metadata.Test plan