Add OpenTelemetry trace context propagation - #99
patrickleet merged 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds trace-context propagation helpers, observability manifest types, runtime tracing spans, and scaffold support for metrics and tracing. It also updates generated GitOps output, docs, and transport tests to carry and validate trace metadata. ChangesObservability and scaffolded tracing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🧹 Nitpick comments (2)
distributed_cli/src/generate/service_crate.rs (1)
82-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the duplicated metrics
mainbranches.The
Http if self.metricsandKnative if self.metricsarms are identical except forroutervscloud_events_router. The body (listener bind,/metricsroute,axum::serve,metrics()handler) could be generated once with the router constructor selected by transport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distributed_cli/src/generate/service_crate.rs` around lines 82 - 132, The metrics-enabled HTTP and Knative branches in service_crate::generate are duplicating the same main/metrics handler logic. Refactor the ServiceTransport::Http if self.metrics and ServiceTransport::Knative if self.metrics arms to share one template, and make only the router constructor differ between distributed::microsvc::router and distributed::microsvc::cloud_events_router while keeping the listener bind, /metrics route, axum::serve call, and metrics() output in one generated body.distributed_cli/src/cli.rs (1)
74-74: 📐 Maintainability & Code Quality | 🔵 TrivialUse
visible_aliasfor the--otelflag to ensure it appears in--help.The
alias = "otel"attribute creates a functional alias that works at runtime but hides it from the help output. Since the README documents--otelas a supported flag, users following the documentation and then running--helpwill not find it listed.- #[arg(long, alias = "otel")] + #[arg(long, visible_alias = "otel")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distributed_cli/src/cli.rs` at line 74, The `--otel` flag alias on the CLI argument is currently hidden from help output because `alias = "otel"` is used in the `cli.rs` argument definition. Update the `#[arg(...)]` on the affected option in `distributed_cli::cli::Cli` to use `visible_alias = "otel"` instead, so the flag remains functional and is shown in `--help`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distributed_cli/src/generate/gitops.rs`:
- Around line 119-131: The OTEL_SERVICE_NAME env var in the gitops template is
using the deploy chart name from .Chart.Name, which produces the wrong service
identity for traces. Update the env block in the generator that builds the Helm
snippet to inject the package/service name already used elsewhere in this
codepath (matching ServiceManifest::new and the service label), instead of
hardcoding .Chart.Name, so trace and metric identities stay aligned.
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 53-55: The generated main entrypoint is missing tracing setup, so
spans are never exported even when the otel feature is enabled. Update
service_crate.rs in main_rs to conditionally emit the opentelemetry OTLP
exporter initialization from OTEL_EXPORTER_OTLP_ENDPOINT and install the
matching tracing_subscriber layer when self.tracing is true. Make sure the
generated main function performs this setup before calling
distributed::microsvc::serve, and keep the logic gated so non-tracing builds
remain unchanged.
In `@src/microsvc/knative_ingress.rs`:
- Around line 96-113: The trace context update in
parse_cloud_event/inject_http_trace_context is unconditional and can wipe
existing metadata from parse_binary or parse_structured when HTTP trace headers
are absent. Add a guard in inject_http_trace_context so it only calls
TraceContext::inject_vec when at least one of TRACEPARENT or TRACESTATE is
present in the HeaderMap, otherwise leave the Message.metadata unchanged.
In `@src/trace_context.rs`:
- Around line 101-109: The is_valid_traceparent validator currently accepts any
non-ff version and uppercase hex, but it should enforce the documented
version-00 shape. Update the checks in is_valid_traceparent (and the related
logic around the traceparent parsing/validation path) to require version == "00"
and to only accept lowercase hex for version-00 fields, while keeping the
existing length and format validation for trace_id, parent_id, and flags.
---
Nitpick comments:
In `@distributed_cli/src/cli.rs`:
- Line 74: The `--otel` flag alias on the CLI argument is currently hidden from
help output because `alias = "otel"` is used in the `cli.rs` argument
definition. Update the `#[arg(...)]` on the affected option in
`distributed_cli::cli::Cli` to use `visible_alias = "otel"` instead, so the flag
remains functional and is shown in `--help`.
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 82-132: The metrics-enabled HTTP and Knative branches in
service_crate::generate are duplicating the same main/metrics handler logic.
Refactor the ServiceTransport::Http if self.metrics and
ServiceTransport::Knative if self.metrics arms to share one template, and make
only the router constructor differ between distributed::microsvc::router and
distributed::microsvc::cloud_events_router while keeping the listener bind,
/metrics route, axum::serve call, and metrics() output in one generated body.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aacf9019-2f39-41e1-8653-f91da7554bfa
📒 Files selected for processing (24)
Cargo.tomldistributed_cli/README.mddistributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/src/lib.rsdocs/observability.mddocs/transports.mdsrc/bus/message.rssrc/bus/runner.rssrc/entity/entity.rssrc/entity/event_record.rssrc/lib.rssrc/manifest.rssrc/microsvc/knative_ingress.rssrc/microsvc/service.rssrc/outbox/message.rssrc/outbox_worker/outbox_dispatch.rssrc/trace_context.rstests/kafka_transport/main.rstests/nats_transport/main.rstests/persistent_repository_conformance/scenario.rstests/rabbitmq_transport/main.rs
b07d1eb to
6d43624
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distributed_cli/README.md`:
- Around line 25-30: Remove the duplicated `--gitops` entry from the options
list in the README help text so the `dctl scaffold` flags are listed only once.
Update the sentence that enumerates common flags around `--gitops`, `--metrics`,
and `--tracing` to keep the list accurate and avoid implying multiple distinct
`--gitops` switches.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad3470a8-d2c1-499f-8dc1-f77e2428607e
📒 Files selected for processing (24)
Cargo.tomldistributed_cli/README.mddistributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/src/lib.rsdocs/observability.mddocs/transports.mdsrc/bus/message.rssrc/bus/runner.rssrc/entity/entity.rssrc/entity/event_record.rssrc/lib.rssrc/manifest.rssrc/microsvc/knative_ingress.rssrc/microsvc/service.rssrc/outbox/message.rssrc/outbox_worker/outbox_dispatch.rssrc/trace_context.rstests/kafka_transport/main.rstests/nats_transport/main.rstests/persistent_repository_conformance/scenario.rstests/rabbitmq_transport/main.rs
✅ Files skipped from review due to trivial changes (3)
- docs/observability.md
- docs/transports.md
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/kafka_transport/main.rs
- distributed_cli/src/cli.rs
- src/outbox_worker/outbox_dispatch.rs
- tests/persistent_repository_conformance/scenario.rs
- src/lib.rs
- distributed_cli/src/lib.rs
- tests/rabbitmq_transport/main.rs
- src/outbox/message.rs
- distributed_cli/src/generate/gitops.rs
- src/entity/entity.rs
- src/microsvc/service.rs
- tests/nats_transport/main.rs
- src/microsvc/knative_ingress.rs
- src/trace_context.rs
- src/bus/runner.rs
- src/bus/message.rs
- distributed_cli/src/generate/service_crate.rs
- distributed_cli/src/generate/mod.rs
- src/entity/event_record.rs
- src/manifest.rs
711ce33 to
258708c
Compare
|
CodeRabbit summary nitpick follow-up ( |
|
CodeRabbit summary nitpick follow-up (duplicated metrics |
258708c to
7782aa9
Compare
|
Rebased and stacked on #100 (base retargeted to
Verification: fmt, clippy (lib all-features + cli all-targets) -D warnings, cargo test --all-features (783 passed), cargo test -p distributed_cli, and scaffold e2e: |
7782aa9 to
6ac1524
Compare
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id codex--opentelemetry-tracing-compatibility@review-fix@1783207849587-be2c
# View logs:
atc logs codex--opentelemetry-tracing-compatibility@review-fix@1783207849587-be2c
# Attach to tmux:
tmux attach -t codex--opentelemetry-tracing-compatibility@review-fix@1783207849587-be2c |
|
Review-fix pass completed and pushed.
Build status:
Prefetched review data showed 0 unresolved threads before this push, so no thread resolution calls were needed. Confidence: 100%. No human-review blockers flagged. |
|
I'll take a look at the updated diff. ᕦ(ò_ó)ᕤ ✅ Action performedReview finished.
|
Implements [[tasks/opentelemetry-tracing-compatibility]]
Implements [[codex/opentelemetry-tracing-compatibility]]
When a dispatch or outbox publish runs inside an already-active local span, applying the remote traceparent unconditionally re-parents the framework span and breaks the local hierarchy. Only extract the remote parent at trace entry (no current span). Found by codex review of #112; lands here because the affected code is this PR's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRjoCyotvAaK3VbhZRd2gq
44ec4e9 to
df34e5e
Compare
The library fix (set_span_parent_from_metadata_if_no_current_span) moved to #99 where the affected code lives; this keeps the regression coverage found by the codex review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/outbox/message.rs (1)
493-521: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
traceparent()/tracestate()are case-sensitive whiletrace_context()is not.
meta()does an exact-caseHashMap::get, buttrace_context()reconstructs viaTraceContext::from_metadata, which matches keys case-insensitively (eq_ignore_ascii_case). If metadata ever contains a non-canonically-casedtraceparent/tracestateentry (e.g. inserted viaset_metaor via a rawHashMappassed tocreate_with_metadata/encode_with_metadatarather than throughset_trace_context),traceparent()/tracestate()silently returnNonewhiletrace_context()still returns a populated value — the two accessors disagree on the same underlying data. Given the PR's ownMessagecarrier deliberately makes trace lookups case-insensitive to tolerate non-canonical header casing from external systems, this HashMap-backed carrier should apply the same guarantee.♻️ Proposed fix: make the single-key getters case-insensitive too
- /// Get the W3C `traceparent`, if set. - pub fn traceparent(&self) -> Option<&str> { - self.meta(TRACEPARENT) - } - - /// Get the W3C `tracestate`, if set. - pub fn tracestate(&self) -> Option<&str> { - self.meta(TRACESTATE) - } + /// Get the W3C `traceparent`, if set. + pub fn traceparent(&self) -> Option<&str> { + self.metadata + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(TRACEPARENT)) + .map(|(_, v)| v.as_str()) + } + + /// Get the W3C `tracestate`, if set. + pub fn tracestate(&self) -> Option<&str> { + self.metadata + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(TRACESTATE)) + .map(|(_, v)| v.as_str()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/outbox/message.rs` around lines 493 - 521, `Message::traceparent()` and `Message::tracestate()` currently use `meta()` for exact-case lookup, which can disagree with `Message::trace_context()`; update the single-key accessors to perform case-insensitive metadata matching like `TraceContext::from_metadata` does. Reuse the existing `Message`/`meta` path or add a helper so `traceparent()`, `tracestate()`, and `trace_context()` all return consistent results regardless of key casing from `set_meta`, `create_with_metadata`, or `encode_with_metadata`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/outbox/message.rs`:
- Around line 493-521: `Message::traceparent()` and `Message::tracestate()`
currently use `meta()` for exact-case lookup, which can disagree with
`Message::trace_context()`; update the single-key accessors to perform
case-insensitive metadata matching like `TraceContext::from_metadata` does.
Reuse the existing `Message`/`meta` path or add a helper so `traceparent()`,
`tracestate()`, and `trace_context()` all return consistent results regardless
of key casing from `set_meta`, `create_with_metadata`, or
`encode_with_metadata`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c9103e8-9ad1-4d9d-b37a-2786c50cda78
📒 Files selected for processing (25)
Cargo.tomldistributed_cli/README.mddistributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/src/lib.rsdistributed_cli/tests/cli_scaffold_compile.rsdocs/observability.mddocs/transports.mdsrc/bus/message.rssrc/bus/runner.rssrc/entity/entity.rssrc/entity/event_record.rssrc/lib.rssrc/manifest.rssrc/microsvc/knative_ingress.rssrc/microsvc/service.rssrc/outbox/message.rssrc/outbox_worker/outbox_dispatch.rssrc/trace_context.rstests/kafka_transport/main.rstests/nats_transport/main.rstests/persistent_repository_conformance/scenario.rstests/rabbitmq_transport/main.rs
💤 Files with no reviewable changes (5)
- tests/persistent_repository_conformance/scenario.rs
- tests/nats_transport/main.rs
- tests/rabbitmq_transport/main.rs
- tests/kafka_transport/main.rs
- src/trace_context.rs
✅ Files skipped from review due to trivial changes (3)
- distributed_cli/README.md
- docs/transports.md
- docs/observability.md
🚧 Files skipped from review as they are similar to previous changes (9)
- distributed_cli/src/lib.rs
- src/bus/runner.rs
- src/entity/event_record.rs
- src/entity/entity.rs
- src/microsvc/service.rs
- src/microsvc/knative_ingress.rs
- src/lib.rs
- src/bus/message.rs
- src/manifest.rs
* test: docker-level observability integration suites - tests/metrics_exposition: real HTTP scrape of /metrics across all framework families, linted with promtool check metrics (PROMTOOL env gate) - tests/otel_export: real OTLP pipeline -> OpenTelemetry Collector container; asserts distributed.microsvc.dispatch arrives parented to the incoming W3C traceparent (endpoint/file env gates) - integration-observability.yaml: reusable workflow running both suites plus helm template + kubeconform validation of scaffolded ServiceMonitor/ PrometheusRule/OTLP-env output against published CRD schemas; wired into PR-quality and push-main pipelines Implements [[tasks/observability-integration-tests]] * test: cover nested span parenting in the OTLP export e2e The library fix (set_span_parent_from_metadata_if_no_current_span) moved to #99 where the affected code lives; this keeps the regression coverage found by the codex review.
a3fba9b
into
codex/prometheus-metrics-endpoint
* Add Prometheus metrics endpoint * fix: address prometheus metrics review feedback Implements [[codex/prometheus-metrics-endpoint]] * Add OpenTelemetry trace context propagation (#99) * feat: add OpenTelemetry trace context propagation Implements [[tasks/opentelemetry-tracing-compatibility]] * fix: resolve tracing scaffold review findings Implements [[codex/opentelemetry-tracing-compatibility]] * fix: preserve local span hierarchy when a parent span is active When a dispatch or outbox publish runs inside an already-active local span, applying the remote traceparent unconditionally re-parents the framework span and breaks the local hierarchy. Only extract the remote parent at trace entry (no current span). Found by codex review of #112; lands here because the affected code is this PR's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRjoCyotvAaK3VbhZRd2gq * test: docker-level observability integration suites (#112) * test: docker-level observability integration suites - tests/metrics_exposition: real HTTP scrape of /metrics across all framework families, linted with promtool check metrics (PROMTOOL env gate) - tests/otel_export: real OTLP pipeline -> OpenTelemetry Collector container; asserts distributed.microsvc.dispatch arrives parented to the incoming W3C traceparent (endpoint/file env gates) - integration-observability.yaml: reusable workflow running both suites plus helm template + kubeconform validation of scaffolded ServiceMonitor/ PrometheusRule/OTLP-env output against published CRD schemas; wired into PR-quality and push-main pipelines Implements [[tasks/observability-integration-tests]] * test: cover nested span parenting in the OTLP export e2e The library fix (set_span_parent_from_metadata_if_no_current_span) moved to #99 where the affected code lives; this keeps the regression coverage found by the codex review. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix: address observability code-review findings - knative ingress: tracestate-only HTTP headers no longer delete the message's existing traceparent (headers win only with a traceparent) - backlog gauges: drop the 5s refresh throttle — refreshes are activity-driven with no timer, so any skipped pass froze the gauges at stale values after the final drain - OutboxStore::backlog_stats default: bounded scan (1000 rows) instead of paging the whole outbox; count saturates, oldest stays exact - span parenting: one rule everywhere — an active local span wins; transport receive now uses the same conditional parenting as dispatch/outbox, documented on the helper - TraceContext::from_metadata: first match wins on duplicate keys, matching Message accessors and OTel span-parent extraction - tests: serialize unknown_command against the global metrics registry Implements [[tasks/observability-prs-rebase]] * fix: restrict scaffolded OTLP protocol Implements [[codex/prometheus-metrics-endpoint]] * chore: prepare telemetry foundation Implements [[tasks/distributed-telemetry-foundation-cleanup-1]] --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
otelfeature that emits framework-owned tracing spans without requiring an OpenTelemetry SDK in the default builddsvc scaffold --metrics --tracing/--otelGitOps output for OTLP env values and HTTP ServiceMonitor generationVerification
cargo fmt --checkgit diff --checkcargo test --all-featurescargo clippy --lib --all-features -- -D warningscargo test -p distributed_clicargo clippy -p distributed_cli --all-targets -- -D warningscargo run -p distributed_cli --bin dsvc -- scaffold observability-orders --path target/tmp/scaffold-observability --store in-memory --transport http --metrics --tracing --gitops --force --distributed-path .cargo check --manifest-path target/tmp/scaffold-observability/Cargo.tomlImplements [[tasks/opentelemetry-tracing-compatibility]].
Summary by CodeRabbit
dctl scaffold:tracing/otel(OTLP-enabled spans with GitOps/Helm env injection) andmetrics(Prometheus/metricsartifacts for HTTP).traceparent/tracestate) propagation across messages, entities, event records, and outbox flows.otelspan configuration.