feat: add W3C Trace Context propagation middleware for OpenTelemetry - #38
Conversation
PR SummaryIntroduces OpenTelemetry-based trace context propagation to correlate commands and event handling.
Written by Cursor Bugbot for commit 91c29a8. This will update automatically on new commits. Configure here. |
|
Warning Rate limit exceeded@yordis has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 31 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces OpenTelemetry W3C Trace Context propagation to Commanded via a new middleware module. It captures OpenTelemetry span context during command dispatch and injects traceparent/tracestate headers into event metadata for downstream trace correlation. Includes documentation, optional dependencies, and comprehensive tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
112e820 to
9991ef8
Compare
a400ee9 to
8240051
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/opentelemetry/trace_context_propagator_test.exs (1)
14-26: Consider case-insensitive hex matching in W3C format validation.The regex pattern uses
[a-f0-9]which only matches lowercase hex digits. The W3C Trace Context specification allows both uppercase and lowercase hex characters in traceparent.🔎 Proposed fix for case-insensitive matching
- assert Regex.match?(~r/^00-[a-f0-9]{32}-[a-f0-9]{16}-[a-f0-9]{2}$/, traceparent) + assert Regex.match?(~r/^00-[a-fA-F0-9]{32}-[a-fA-F0-9]{16}-[a-fA-F0-9]{2}$/i, traceparent)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
guides/explanations/fork-differences.mdlib/commanded/middleware/trace_context_propagator.exmix.exstest/opentelemetry/trace_context_propagator_test.exstest/support/opentelemetry_case.ex
🚧 Files skipped from review as they are similar to previous changes (2)
- mix.exs
- test/support/opentelemetry_case.ex
🧰 Additional context used
🧬 Code graph analysis (1)
test/opentelemetry/trace_context_propagator_test.exs (1)
lib/commanded/middleware/trace_context_propagator.ex (1)
before_dispatch(45-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Assurance (1.18.x, 27)
🔇 Additional comments (8)
lib/commanded/middleware/trace_context_propagator.ex (4)
1-1: Conditional compilation approach is appropriate for optional dependencies.The compile-time check ensures the module is only defined when the OpenTelemetry propagator is available. Users who configure this middleware without installing
opentelemetry_apiwill encounter a clear module-not-found error, which is acceptable for explicit opt-in features.
57-60: LGTM!The helper functions correctly handle the return values from
List.keyfind/3, cleanly separating the nil case (key not found) from the tuple case (key found with value).
62-64: LGTM!The passthrough implementations are appropriate since the middleware only needs to capture trace context during
before_dispatch/1. No additional work is required after dispatch or on failure.
45-55: No action needed. The implementation correctly uses the OpenTelemetry API. Theinject/1function with an empty list is the correct API foropentelemetry_apiversion 1.0.0, and the returned headers use string keys for "traceparent" and "tracestate" as expected. Themaybe_assign/3helper properly handles both nil results (when headers are not found) and{key, value}tuples fromList.keyfind/3. Existing tests confirm this works correctly.test/opentelemetry/trace_context_propagator_test.exs (3)
1-11: LGTM!The test setup is appropriate. Using
async: falseis necessary because OpenTelemetry's tracer context is global state that cannot be safely shared across concurrent tests.
28-55: LGTM!The tests comprehensively cover the middleware behavior:
- Correctly verifies no traceparent when no active span
- Validates existing metadata preservation
- Confirms empty tracestate is not added unnecessarily
58-72: LGTM!The tests correctly verify that
after_dispatch/1andafter_failure/1return the pipeline unchanged, confirming the passthrough behavior.guides/explanations/fork-differences.md (1)
150-173: LGTM!The documentation is comprehensive and well-structured. It clearly explains:
- What the feature does
- How to use it (with code example)
- The benefits for distributed tracing
- That it's non-invasive and only activates when spans are present
The format is consistent with other feature documentation in this guide.
8240051 to
0a19480
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
0a19480 to
91c29a8
Compare
No description provided.