feat: Prometheus metrics endpoint and OTel trace propagation - #100
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds trace-context propagation, Prometheus metrics, observability-aware scaffold generation, and matching docs, tests, and CI workflow updates. ChangesObservability runtime
Estimated code review effort: 5 (Critical) | ~120 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 |
c455d62 to
736a894
Compare
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id codex--prometheus-metrics-endpoint@review-fix@1782355524746-27a1
# View logs:
atc logs codex--prometheus-metrics-endpoint@review-fix@1782355524746-27a1
# Attach to tmux:
tmux attach -t codex--prometheus-metrics-endpoint@review-fix@1782355524746-27a1 |
736a894 to
bd57d20
Compare
bd57d20 to
794c272
Compare
|
Rebased onto main (post #105/#106/#111 refactors). Non-mechanical ports:
Verification: cargo fmt --check, clippy --lib --all-features -D warnings, clippy -p distributed_cli --all-targets, cargo test --all-features (763 passed), cargo test -p distributed_cli (39 passed). |
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 (1)
distributed_cli/src/generate/gitops.rs (1)
182-212: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
metrics/serviceMonitor/prometheusRulevalues.yaml keys are emitted regardless of transport, but the templates that consume them only exist for HTTP.
gitops_deploy_values_yamlgates the metrics block only onself.metrics == Some(MetricsTarget::Prometheus)(line 187), butgitops_files()(lines 43-52) only emitsservicemonitor.yaml/prometheusrule.yamlforServiceTransport::Http. For a Knative +--metrics prometheusscaffold,values.yamlwill still advertiseserviceMonitor:/prometheusRule:toggles (confirmed disabled-by-default byknative_metrics_does_not_emit_service_monitorinmod.rs, which only checks template absence, not values.yaml content) that have no corresponding Helm template to act on them — dead, misleading GitOps configuration that could confuse operators togglingserviceMonitor.enabled: trueexpecting a scrape target to appear.🔧 Suggested fix
- let metrics = if self.metrics == Some(MetricsTarget::Prometheus) { + let metrics = if self.metrics == Some(MetricsTarget::Prometheus) + && self.transport == ServiceTransport::Http + { r#"metrics: enabled: true path: /metrics portName: http serviceMonitor: enabled: false interval: 30s scrapeTimeout: 10s prometheusRule: enabled: false "# } else { "" };🤖 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/gitops.rs` around lines 182 - 212, The GitOps values generator is emitting Prometheus-related keys even when the selected transport has no matching Helm templates. Update gitops_deploy_values_yaml in gitops.rs so the metrics/serviceMonitor/prometheusRule block is only included when both self.metrics is Prometheus and the transport is Http, matching the template gating in gitops_files. Keep the existing image/service/bus output unchanged, and make sure the generated values.yaml no longer advertises serviceMonitor or prometheusRule for Knative scaffolds.
🧹 Nitpick comments (6)
src/bus/runner.rs (1)
74-150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSettlement-call failures (ack/nack/dead_letter/park) aren't recorded as transport failures.
recv_nextrecords a metric whensource.recv()fails, but the.ack()/.nack()/.dead_letter()/.park()calls throughoutrun_sourceare bare.await?— if the settle call itself errors (e.g. transient broker/DB error while acking), the error propagates without anyrecord_transport_failurecall. Only the original message failure gets recorded; a failure in the act of settling it is invisible to the metrics this PR just added, and to the generatedDistributedTransportRetryingalert that depends ondistributed_transport_failures_total.Consider a small helper mirroring
recv_nextto wrap settle calls and record on error before propagating.♻️ Sketch of a settle-wrapping helper
+async fn settle_and_record<F, Fut>( + service: Option<&str>, + transport: &str, + kind: MessageKind, + outcome: &'static str, + settle: F, +) -> Result<(), TransportError> +where + F: FnOnce() -> Fut, + Fut: std::future::Future<Output = Result<(), TransportError>>, +{ + match settle().await { + Ok(()) => { + record_transport_message(service, transport, kind, outcome); + Ok(()) + } + Err(error) => { + record_transport_failure(service, transport, error.kind(), "settle_error"); + Err(error) + } + } +}Also applies to: 152-164
🤖 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/bus/runner.rs` around lines 74 - 150, Settlement calls in run_source are not emitting transport-failure metrics when ack/nack/dead_letter/park itself fails, so wrap those settle operations with a helper similar to recv_next that records via record_transport_failure before returning the error. Update the existing settle paths in run_source (including the FailureAction branches and the successful dispatch ack) to use that helper, using the same service/transport/kind context and the settle action name so broker-side settlement errors are counted consistently.src/metrics.rs (1)
206-259: 🚀 Performance & Scalability | 🔵 TrivialSingle global mutex serializes all metric writes.
MetricsRegistryguards every counter/histogram/gauge family behind oneMutex<MetricsInner>(Line 207-209, 437-441), so a busy microsvc dispatch loop, bus runner settlement, and outbox worker all contend on the same lock for every recorded event. Critical sections are small, so this is likely fine at moderate load, but worth watching as a scalability boundary if dispatch/transport throughput grows (e.g. sharding by metric family, or using atomics for simple counters).Also applies to: 437-442
🤖 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/metrics.rs` around lines 206 - 259, MetricsRegistry currently uses one global Mutex<MetricsInner> for all metric families, so hot paths like record_microsvc_dispatch, record_transport_message, record_transport_failure, and record_outbox_messages all contend on the same lock. Refactor MetricsRegistry to reduce lock contention by splitting the shared state into smaller per-family locks or using atomics for simple counters while keeping Histograms and service_info synchronized separately. Keep the existing behavior of the record_* methods and MetricsRegistry::inner access, but move the high-frequency updates off the single coarse-grained mutex.src/microsvc/http.rs (1)
49-55: 🔒 Security & Privacy | 🔵 TrivialConsider documenting/enforcing network-level restriction of
/metrics.The new endpoint is unauthenticated by design (typical for Prometheus scraping), but Prometheus's own security guidance is explicit that
/metrics"should not be exposed to publicly accessible networks like the internet" without additional protection. Since the PR description mentions GitOps/ObserveStack scrape behavior is documented, ensure the docs/scaffold also call out that this route must stay behind a private network/ingress policy rather than a public listener.Also applies to: 75-79
🤖 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/microsvc/http.rs` around lines 49 - 55, The unauthenticated /metrics route added in Router::new should be documented and enforced as private-only, since it must not be exposed on a public listener. Update the HTTP scaffold/docs around metrics_handler and the Router setup to clearly state that Prometheus scraping must happen behind a private network, ingress policy, or equivalent network restriction. If there is a configuration or deployment helper associated with the metrics feature, add or strengthen the guard there so /metrics cannot be enabled on a publicly reachable endpoint by default.distributed_cli/tests/cli_scaffold.rs (1)
87-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
scaffold()helper here. It already covers tmp-dir setup,--distributed-path, and stderr-rich failure reporting; this test can pass--metrics prometheusthrough that helper instead of duplicating theCommand::new(...).status()path.🤖 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/tests/cli_scaffold.rs` around lines 87 - 121, The test scaffold_metrics_prometheus_emits_operator_resources is duplicating the manual Command::new(...).status() setup instead of using the shared scaffold() helper. Update this test to call scaffold() with the same arguments, passing --metrics prometheus through the helper while keeping the existing assertions for generated files and Cargo.toml, so tmp-dir handling, --distributed-path wiring, and stderr-rich failure reporting stay centralized.distributed_cli/src/generate/mod.rs (2)
414-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting values.yaml has no dead metrics/serviceMonitor keys for Knative.
This test only checks template file absence for Knative + metrics, but doesn't assert on
values.yamlcontents. See the related comment ongitops.rsgitops_deploy_values_yaml— if that function isn't transport-gated, this test would miss catching the resulting dead configuration.🤖 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/mod.rs` around lines 414 - 425, Extend the knative_metrics_does_not_emit_service_monitor test to also verify the generated values.yaml for the ServiceTransport::Knative path, not just the absence of .gitops/deploy/templates/servicemonitor.yaml and prometheusrule.yaml. Inspect the generated project from generate_service_scaffold and assert that any metrics/serviceMonitor-related keys are not present in the values.yaml output, so the test covers the gitops_deploy_values_yaml behavior and catches dead config for Knative.
385-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeak assertion doesn't verify metrics-specific behavior.
assert!(service.contains(".named(\"orders\")"))at line 399 checks a call that's unconditional inservice_crate.rs(Arc::new(Service::new().named({service_name}).routes(routes))is emitted regardless ofself.metrics). This assertion passes even without metrics enabled, so it doesn't actually validate anything metrics-specific in this test.♻️ Suggested cleanup
- let service = contents(&project, "src/service.rs"); - assert!(service.contains(".named(\"orders\")")); - let values = contents(&project, ".gitops/deploy/values.yaml");🤖 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/mod.rs` around lines 385 - 413, The test in generate_service_scaffold is asserting an unconditional service name setup, so it does not verify any metrics-specific behavior. Update the gitops_http_metrics_emits_prometheus_operator_resources test to assert a metrics-related detail from src/service.rs or the generated GitOps templates/values that only appears when MetricsTarget::Prometheus is enabled, and remove the generic .named("orders") check. Use the generate_service_scaffold and contents helpers to target a metric-specific symbol or config entry that proves the metrics path was generated.
🤖 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 `@src/outbox_worker/outbox_dispatch.rs`:
- Around line 267-303: The backlog gauge refresh in
record_outbox_backlog()/record_backlog_gauges() is doing an expensive full
pending-row scan on every dispatch and publish path. Replace the
store.pending(BACKLOG_SAMPLE_LIMIT) read with a lightweight OutboxStore query
that returns only the backlog count and oldest created_at (for example via COUNT
and MIN(created_at)), and have record_backlog_gauges use that smaller result to
call set_outbox_backlog. If possible, also add throttling so
BusOutboxPublishHook::record_outbox_outcomes and
OutboxDispatch::record_outbox_backlog do not recompute gauges on every
invocation.
---
Outside diff comments:
In `@distributed_cli/src/generate/gitops.rs`:
- Around line 182-212: The GitOps values generator is emitting
Prometheus-related keys even when the selected transport has no matching Helm
templates. Update gitops_deploy_values_yaml in gitops.rs so the
metrics/serviceMonitor/prometheusRule block is only included when both
self.metrics is Prometheus and the transport is Http, matching the template
gating in gitops_files. Keep the existing image/service/bus output unchanged,
and make sure the generated values.yaml no longer advertises serviceMonitor or
prometheusRule for Knative scaffolds.
---
Nitpick comments:
In `@distributed_cli/src/generate/mod.rs`:
- Around line 414-425: Extend the knative_metrics_does_not_emit_service_monitor
test to also verify the generated values.yaml for the ServiceTransport::Knative
path, not just the absence of .gitops/deploy/templates/servicemonitor.yaml and
prometheusrule.yaml. Inspect the generated project from
generate_service_scaffold and assert that any metrics/serviceMonitor-related
keys are not present in the values.yaml output, so the test covers the
gitops_deploy_values_yaml behavior and catches dead config for Knative.
- Around line 385-413: The test in generate_service_scaffold is asserting an
unconditional service name setup, so it does not verify any metrics-specific
behavior. Update the gitops_http_metrics_emits_prometheus_operator_resources
test to assert a metrics-related detail from src/service.rs or the generated
GitOps templates/values that only appears when MetricsTarget::Prometheus is
enabled, and remove the generic .named("orders") check. Use the
generate_service_scaffold and contents helpers to target a metric-specific
symbol or config entry that proves the metrics path was generated.
In `@distributed_cli/tests/cli_scaffold.rs`:
- Around line 87-121: The test
scaffold_metrics_prometheus_emits_operator_resources is duplicating the manual
Command::new(...).status() setup instead of using the shared scaffold() helper.
Update this test to call scaffold() with the same arguments, passing --metrics
prometheus through the helper while keeping the existing assertions for
generated files and Cargo.toml, so tmp-dir handling, --distributed-path wiring,
and stderr-rich failure reporting stay centralized.
In `@src/bus/runner.rs`:
- Around line 74-150: Settlement calls in run_source are not emitting
transport-failure metrics when ack/nack/dead_letter/park itself fails, so wrap
those settle operations with a helper similar to recv_next that records via
record_transport_failure before returning the error. Update the existing settle
paths in run_source (including the FailureAction branches and the successful
dispatch ack) to use that helper, using the same service/transport/kind context
and the settle action name so broker-side settlement errors are counted
consistently.
In `@src/metrics.rs`:
- Around line 206-259: MetricsRegistry currently uses one global
Mutex<MetricsInner> for all metric families, so hot paths like
record_microsvc_dispatch, record_transport_message, record_transport_failure,
and record_outbox_messages all contend on the same lock. Refactor
MetricsRegistry to reduce lock contention by splitting the shared state into
smaller per-family locks or using atomics for simple counters while keeping
Histograms and service_info synchronized separately. Keep the existing behavior
of the record_* methods and MetricsRegistry::inner access, but move the
high-frequency updates off the single coarse-grained mutex.
In `@src/microsvc/http.rs`:
- Around line 49-55: The unauthenticated /metrics route added in Router::new
should be documented and enforced as private-only, since it must not be exposed
on a public listener. Update the HTTP scaffold/docs around metrics_handler and
the Router setup to clearly state that Prometheus scraping must happen behind a
private network, ingress policy, or equivalent network restriction. If there is
a configuration or deployment helper associated with the metrics feature, add or
strengthen the guard there so /metrics cannot be enabled on a publicly reachable
endpoint by default.
🪄 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: 118ac747-07a6-4a24-aa9c-2d00f43090c7
📒 Files selected for processing (43)
Cargo.tomlREADME.mddistributed_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.rsdocs/metrics.mdsrc/bus/in_memory_bus.rssrc/bus/kafka.rssrc/bus/mod.rssrc/bus/nats.rssrc/bus/rabbitmq.rssrc/bus/runner.rssrc/bus/source.rssrc/bus/sql_bus_common.rssrc/commit_builder/mod.rssrc/lib.rssrc/metrics.rssrc/microsvc/http.rssrc/microsvc/knative_ingress.rssrc/microsvc/service.rssrc/outbox_worker/outbox_dispatch.rssrc/outbox_worker/outbox_source.rssrc/outbox_worker/publish_hook.rssrc/postgres_repo/mod.rssrc/sqlite_repo/mod.rssrc/sqlx_repo/repo.rstests/distributed_read_model/checkout_saga_service/mod.rstests/distributed_read_model/main.rstests/distributed_read_model/seat_inventory_service/mod.rstests/distributed_read_model_board/board_service/mod.rstests/distributed_read_model_board/main.rstests/knative_cloudevents/main.rstests/microsvc/handlers/mod.rstests/microsvc/transport_http.rstests/queued_repo/main.rstests/sagas/handlers/inventory/mod.rstests/sagas/handlers/orders/mod.rstests/sagas/handlers/payments/mod.rstests/sagas/handlers/saga/mod.rs
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id codex--prometheus-metrics-endpoint@review-fix@1783207832567-278a
# View logs:
atc logs codex--prometheus-metrics-endpoint@review-fix@1783207832567-278a
# Attach to tmux:
tmux attach -t codex--prometheus-metrics-endpoint@review-fix@1783207832567-278a |
Implements [[codex/prometheus-metrics-endpoint]]
Review Fix Summary
Build status:
Confidence: 100% |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== OutboxBacklogStats ==="
rg -n "OutboxBacklogStats" --type=rust -C2
echo "=== BACKLOG_SAMPLE_LIMIT / throttling ==="
rg -n "BACKLOG_SAMPLE_LIMIT|throttl" --type=rust -i -C2 src/outbox_worker src/metrics.rs
echo "=== Knative metrics gating in gitops.rs ==="
rg -n "MetricsTarget|Knative" distributed_cli/src/generate/gitops.rs -C3
echo "=== settle failure recording ==="
rg -n "settle_" src/bus/runner.rs -C3
echo "=== per-family locks in metrics.rs ==="
rg -n "Mutex|RwLock" src/metrics.rs -C1
echo "=== docs on /metrics exposure ==="
rg -n "private network|ingress|unauthenticated" docs/metrics.md -C2
echo "=== sqlite MIN(created_at) numeric fix ==="
rg -n "MIN\(created_at\)|CAST" src/sqlite_repo -C3Length of output: 1084 🏁 Script executed: #!/bin/bash
set +e
echo "=== OutboxBacklogStats ==="
rg -n "OutboxBacklogStats" --type=rust -C2 || true
echo "=== BACKLOG_SAMPLE_LIMIT / throttling ==="
rg -n "BACKLOG_SAMPLE_LIMIT|throttl" --type=rust -i -C2 src/outbox_worker src/metrics.rs || true
echo "=== Knative metrics gating in gitops.rs ==="
rg -n "MetricsTarget|Knative" distributed_cli/src/generate/gitops.rs -C3 || true
echo "=== settle failure recording ==="
rg -n "settle_" src/bus/runner.rs -C3 || true
echo "=== per-family locks in metrics.rs ==="
rg -n "Mutex|RwLock" src/metrics.rs -C1 || true
echo "=== docs on /metrics exposure ==="
rg -n "private network|ingress|unauthenticated" docs/metrics.md -C2 || true
echo "=== sqlite MIN(created_at) numeric fix ==="
rg -n "MIN\(created_at\)|CAST" src/sqlite_repo -C3 || true
echo "=== files present check ==="
fd -e rs . src/outbox_worker src/sqlite_repoLength of output: 1041 🏁 Script executed: #!/bin/bash
set +e
echo "=== pwd/branch ==="
pwd
git branch --show-current
git log --oneline -5
echo "=== repo root listing ==="
ls -la
echo "=== find outbox_dispatch.rs ==="
fd outbox_dispatch.rs
echo "=== find metrics.rs ==="
fd metrics.rs
echo "=== find sqlite repo dir ==="
fd -t d sqlite_repoLength of output: 2612 🏁 Script executed: #!/bin/bash
set +e
echo "=== OutboxBacklogStats ==="
rg -n "OutboxBacklogStats" -g '*.rs' -C2
echo "=== BACKLOG_SAMPLE_LIMIT / throttl ==="
rg -n "BACKLOG_SAMPLE_LIMIT|throttl" -i -g '*.rs' -C2 src/outbox_worker src/metrics.rs
echo "=== gitops.rs metrics gating ==="
rg -n "MetricsTarget|Knative|Transport::" distributed_cli/src/generate/gitops.rs -C3
echo "=== settle failure recording in runner.rs ==="
rg -n "settle_" src/bus/runner.rs -C3
echo "=== locks in metrics.rs ==="
rg -n "Mutex|RwLock" src/metrics.rs -C1
echo "=== docs exposure note ==="
rg -n -i "private network|ingress|unauthenticated|expose" docs/metrics.md -C2
echo "=== sqlite backlog query ==="
rg -n -i "MIN\(|created_at" src/sqlite_repo -C3Length of output: 1055 🏁 Script executed: #!/bin/bash
rg --version
echo "---test1---"
rg -n "fn " src/metrics.rs | head -20
echo "---test2---"
rg -n "OutboxBacklogStats" src/ -rLength of output: 2111 🏁 Script executed: #!/bin/bash
echo "=== OutboxBacklogStats search ==="
rg -n "OutboxBacklogStats" src/ || echo "NOT FOUND"
echo ""
echo "=== outbox_dispatch.rs full content (backlog gauge section) ==="
rg -n "fn.*backlog|BACKLOG|pending\(" src/outbox_worker/outbox_dispatch.rs -C2
echo ""
echo "=== throttl in metrics.rs or outbox files ==="
rg -n -i "throttl" src/metrics.rs src/outbox_worker/*.rs
echo ""
echo "=== gitops.rs metrics ==="
rg -n "MetricsTarget|Knative" distributed_cli/src/generate/gitops.rs -C2
echo ""
echo "=== settle_ in runner.rs ==="
rg -n "settle_" src/bus/runner.rs
echo ""
echo "=== Mutex/RwLock in metrics.rs ==="
rg -n "Mutex|RwLock" src/metrics.rs
echo ""
echo "=== metrics.md exposure ==="
rg -n -i "private|ingress|unauthenticated|expose|network" docs/metrics.md
echo ""
echo "=== sqlite backlog query ==="
rg -n -i "created_at" src/sqlite_repo/*.rsLength of output: 11422 All confirmed — every claim checks out against the code:
Nice work — thanks for the thorough summary and verification steps (fmt/clippy/tests all passing). [resolve] |
* 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>
- 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]]
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)
src/outbox/message.rs (1)
493-521: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
traceparent()/tracestate()should use the same case-insensitive lookup astrace_context()
OutboxMessageaccepts arbitrary metadata, so mixed-case trace keys are reachable viacreate_with_metadata,encode_with_metadata, orset_meta.trace_context()already reads them case-insensitively, but these getters use exactHashMap::get, which can make the same message report different trace values. Reuse the same lookup path here.🤖 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, `OutboxMessage::traceparent()` and `OutboxMessage::tracestate()` are using exact metadata key lookup, which can miss mixed-case trace keys and disagree with `trace_context()`. Update these getters to reuse the same case-insensitive metadata lookup path used by `TraceContext::from_metadata` (or a shared helper in `OutboxMessage`), while leaving `meta`, `correlation_id`, and `causation_id` behavior unchanged.src/microsvc/service.rs (1)
1221-1250: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the shared unknown-command metrics series in
dispatch_request_error_codes
dispatch_request_error_codesdispatches"unknown"on the unnamed service, so it hits the same{service="unnamed", message_kind="command", message="unknown", status="unknown_command"}series as the exact-count test above. Addcrate::metrics::async_lock_for_tests()here too, or give this test a distinct service name, to avoid flaky parallel-test failures.🤖 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/microsvc/service.rs` around lines 1221 - 1250, The shared unknown-command metrics series in dispatch_request_error_codes can collide with the existing unknown-command test because both emit the same bounded Prometheus label set. Update the test to either acquire crate::metrics::async_lock_for_tests() before resetting/asserting metrics, or construct the service with a distinct name via test_service/test_routes so it uses a different series. Keep the assertion focused on dispatch and the unknown_command metric output, but ensure the series is isolated from metrics_bucket_unknown_command_under_fixed_message_label.
🧹 Nitpick comments (1)
src/trace_context.rs (1)
86-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the "don't reparent when a span is active" branch.
set_span_parent_from_metadata_if_no_current_spanis the function that implements the PR's key design guarantee (ambient span wins over remote metadata). Theotel-gated tests only coverextract_otel_context_from_metadatadirectly; none enters an activetracing::Spanfirst and asserts thatset_span_parent_from_metadata_if_no_current_spanis a no-op in that case. A regression here would silently break the "local span hierarchy is preserved" guarantee called out in the doc comment (lines 75-84).Also applies to: 309-331
🤖 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/trace_context.rs` around lines 86 - 93, Add a test for the active-span branch in set_span_parent_from_metadata_if_no_current_span to verify it does not reparent when tracing::Span::current() is already set. Create a test that enters an ambient span first, then calls set_span_parent_from_metadata_if_no_current_span with metadata and asserts the span parent remains the local/current span rather than the remote metadata. Keep the focus on the no-op behavior of set_span_parent_from_metadata_if_no_current_span and the surrounding otel-gated tracing context tests.
🤖 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 118-138: The tracing_env_yaml helper is exposing
OTEL_EXPORTER_OTLP_PROTOCOL as a free chart value even though the compiled
tracing setup only supports grpc-tonic. Update tracing_env_yaml to either
hardcode/restrict the protocol to grpc-compatible behavior or expand the tracing
build/config so http/protobuf is actually supported; make the choice consistent
with SpanExporter::builder().build()? and the observability.tracing.* values.
---
Outside diff comments:
In `@src/microsvc/service.rs`:
- Around line 1221-1250: The shared unknown-command metrics series in
dispatch_request_error_codes can collide with the existing unknown-command test
because both emit the same bounded Prometheus label set. Update the test to
either acquire crate::metrics::async_lock_for_tests() before resetting/asserting
metrics, or construct the service with a distinct name via
test_service/test_routes so it uses a different series. Keep the assertion
focused on dispatch and the unknown_command metric output, but ensure the series
is isolated from metrics_bucket_unknown_command_under_fixed_message_label.
In `@src/outbox/message.rs`:
- Around line 493-521: `OutboxMessage::traceparent()` and
`OutboxMessage::tracestate()` are using exact metadata key lookup, which can
miss mixed-case trace keys and disagree with `trace_context()`. Update these
getters to reuse the same case-insensitive metadata lookup path used by
`TraceContext::from_metadata` (or a shared helper in `OutboxMessage`), while
leaving `meta`, `correlation_id`, and `causation_id` behavior unchanged.
---
Nitpick comments:
In `@src/trace_context.rs`:
- Around line 86-93: Add a test for the active-span branch in
set_span_parent_from_metadata_if_no_current_span to verify it does not reparent
when tracing::Span::current() is already set. Create a test that enters an
ambient span first, then calls set_span_parent_from_metadata_if_no_current_span
with metadata and asserts the span parent remains the local/current span rather
than the remote metadata. Keep the focus on the no-op behavior of
set_span_parent_from_metadata_if_no_current_span and the surrounding otel-gated
tracing context tests.
🪄 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: 91bfe17f-ccb1-465a-846f-016626c672e8
📒 Files selected for processing (41)
.github/workflows/integration-observability.yaml.github/workflows/on-pr-quality.yaml.github/workflows/on-push-main-version-and-tag.yamlCargo.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.rsdistributed_cli/tests/cli_scaffold_compile.rsdocs/metrics.mddocs/observability.mddocs/transports.mdsrc/bus/message.rssrc/bus/runner.rssrc/entity/entity.rssrc/entity/event_record.rssrc/lib.rssrc/manifest.rssrc/metrics.rssrc/microsvc/http.rssrc/microsvc/knative_ingress.rssrc/microsvc/service.rssrc/outbox/message.rssrc/outbox_worker/mod.rssrc/outbox_worker/outbox_dispatch.rssrc/outbox_worker/store.rssrc/postgres_repo/mod.rssrc/sqlite_repo/mod.rssrc/sqlx_repo/repo.rssrc/trace_context.rstests/kafka_transport/main.rstests/metrics_exposition/main.rstests/nats_transport/main.rstests/otel_export/collector-config.yamltests/otel_export/main.rstests/persistent_repository_conformance/scenario.rstests/rabbitmq_transport/main.rstests/sqlite_repository/main.rs
✅ Files skipped from review due to trivial changes (6)
- src/outbox_worker/mod.rs
- docs/transports.md
- docs/observability.md
- src/postgres_repo/mod.rs
- distributed_cli/README.md
- docs/metrics.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/microsvc/http.rs
- distributed_cli/src/lib.rs
- distributed_cli/tests/cli_scaffold.rs
- distributed_cli/src/cli.rs
- src/bus/runner.rs
- src/metrics.rs
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id codex--prometheus-metrics-endpoint@review-fix@1783357971719-13d7
# View logs:
atc logs codex--prometheus-metrics-endpoint@review-fix@1783357971719-13d7
# Attach to tmux:
tmux attach -t codex--prometheus-metrics-endpoint@review-fix@1783357971719-13d7 |
Implements [[codex/prometheus-metrics-endpoint]]
|
Review fix summary:
Build status:
Confidence: 100% |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set +e
echo "=== otlpProtocol references ==="
rg -n -i "otlpProtocol|otlp_protocol" -g '*.rs' -g '*.md' .
echo "=== OTEL_EXPORTER_OTLP_PROTOCOL / grpc hardcode ==="
rg -n -i "OTEL_EXPORTER_OTLP_PROTOCOL|grpc" distributed_cli/src/generate/*.rs
echo "=== regression tests referencing otlpProtocol removal ==="
rg -n -i "otlpProtocol|grpc" distributed_cli/tests/*.rsLength of output: 2078
This correctly eliminates the previously exposed knob that could advertise an unsupported HTTP transport. Nice work. [resolve] |
Implements [[tasks/distributed-telemetry-foundation-cleanup-1]]
Now that the observability generators are on main (#100), the CI skill documents ServiceMonitor/PrometheusRule gating and the OTLP env values. Implements [[tasks/cli-skills-init]]
…tributed/ (#113) * feat(cli): add dctl skills init/list — embedded agent skills Materializes agent skills (distributed-usage, distributed-ci, distributed-schema) embedded via include_str! into .distributed/skills/, with harness wiring adapters: .claude/skills/ copies for Claude Code and .agents/skills/ copies + a sentinel-managed AGENTS.md block for Codex, Grok, Gemini, Pi, and AGENTS.md-only tools. Pure generation returns GeneratedProject; per-file drift semantics (created/unchanged/skipped/ updated) make re-runs idempotent and never clobber local edits without --force. Implements [[tasks/cli-skills-init]] * fix: clarify skills init upgrade behavior Implements [[feat/cli-skills-init]] * refactor(cli): wire harness skill locations as symlinks to .distributed One on-disk copy: canonical skills stay under <container>/skills/; each wired harness location (.claude/skills/<name>, .agents/skills/<name>) becomes a relative per-skill symlink to the canonical folder, coexisting with user-owned skills. Non-unix platforms fall back to real copies. A non-link path at a harness location (stale link or old copy layout) is skipped with a warning and converted with --force. Implements [[tasks/cli-skills-init]] * docs(skills): cover --metrics prometheus and --tracing in distributed-ci Now that the observability generators are on main (#100), the CI skill documents ServiceMonitor/PrometheusRule gating and the OTLP env values. Implements [[tasks/cli-skills-init]] * docs(skills): lead distributed-usage with the models-and-handlers thesis The main point of using Distributed: the authored surface is models and handlers; the framework and dctl generate the deterministic structure around them. Emphasized in the skill body and its trigger description. Implements [[tasks/cli-skills-init]] * docs(skills): prefer the highest-level macro APIs in distributed-usage #[sourced] over #[digest]+aggregate!(), the derives over hand plumbing, routes! and with_bus(..).run(..) over manual wiring — dropping a level is a deliberate choice, not a default. Implements [[tasks/cli-skills-init]]
Summary
Verification
Summary by CodeRabbit
GET /metricsfor HTTP and Knative.dctl scaffoldnow supports--metrics prometheuswith GitOps Helm templates (ServiceMonitor/PrometheusRule).--oteland OTLP, including W3C trace-context propagation.