feat(scout): give the on-host discovery agent a /metrics endpoint#3539
Open
chet wants to merge 1 commit into
Open
feat(scout): give the on-host discovery agent a /metrics endpoint#3539chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕮ(•ᴥ•)ᕭ ✅ Action performedFull review finished. |
Contributor
Summary by CodeRabbit
WalkthroughScout adds an optional metrics/health endpoint, defines Prometheus-style action and stream metrics, and emits lifecycle and action outcomes. Log-error conversion failures are now propagated instead of only being logged. ChangesScout metrics instrumentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Options
participant ScoutService
participant MetricsEndpoint
participant ActionLoop
participant carbide_instrument
Options->>ScoutService: metrics_listen_addr
ScoutService->>MetricsEndpoint: start metrics endpoint
ActionLoop->>ActionLoop: handle_action
ActionLoop->>carbide_instrument: emit ScoutActionHandled with action and outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/scout/src/main.rs (1)
194-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured fields for endpoint logs.
Line 195 and Line 197 interpolate queryable values into messages. Emit
addressanderroras tracing fields instead.Proposed fix
- tracing::info!("Spawning metrics endpoint on {}", metrics_config.address); + tracing::info!(address = %metrics_config.address, "spawning metrics endpoint"); if let Err(e) = metrics_endpoint::run_metrics_endpoint(&metrics_config).await { - tracing::error!("Metrics endpoint error: {e}"); + tracing::error!(error = %e, "metrics endpoint error"); }As per coding guidelines, “Use plain
tracing::macros with structured fields for ordinary logging.” As per path instructions, use “structured tracing fields.”🤖 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 `@crates/scout/src/main.rs` around lines 194 - 198, Update the metrics endpoint logs inside the tokio::spawn closure to use structured tracing fields: emit metrics_config.address as an address field in the info event and the endpoint failure as an error field in the error event, while keeping the existing log messages and behavior.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/scout/src/stream.rs`:
- Around line 92-97: Update the stream-establishment error path around
client.scout_stream(request_stream).await so it emits ScoutStreamConnection with
Outcome::Error before propagating the failure. Preserve the existing
reconnect-loop behavior and the create_forge_client metric handling.
---
Nitpick comments:
In `@crates/scout/src/main.rs`:
- Around line 194-198: Update the metrics endpoint logs inside the tokio::spawn
closure to use structured tracing fields: emit metrics_config.address as an
address field in the info event and the endpoint failure as an error field in
the error event, while keeping the existing log messages and behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bbf53f66-4e05-4c5e-b39c-1617db7bf117
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/scout/Cargo.tomlcrates/scout/src/cfg/command_line.rscrates/scout/src/main.rscrates/scout/src/metrics.rscrates/scout/src/stream.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/scout/src/cfg/command_line.rs (1)
111-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winType the metrics listen address as
SocketAddrat the CLI boundary. Both sites treat the metrics address as a bare string, pushing validation from CLI parsing into runtime service startup instead of letting clap/the compiler catch a malformed address immediately.
crates/scout/src/cfg/command_line.rs#L111-L117: changepub metrics_listen_addr: Option<String>topub metrics_listen_addr: Option<SocketAddr>; clap derives parsing fromSocketAddr'sFromStrautomatically, giving a clear CLI-level error on malformed input.crates/scout/src/main.rs#L179-L182: drop the manualaddr_str.parse()/eyre::eyre!("Invalid --metrics-listen-addr...")block and useconfig.metrics_listen_addrdirectly as the already-typedSocketAddr.🤖 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 `@crates/scout/src/cfg/command_line.rs` around lines 111 - 117, The metrics listen address is validated too late as a string. In crates/scout/src/cfg/command_line.rs:111-117, change CommandLine’s metrics_listen_addr field to Option<SocketAddr> and ensure the SocketAddr type is imported; in crates/scout/src/main.rs:179-182, remove the manual addr_str.parse() and custom invalid-address error handling, passing config.metrics_listen_addr directly to the metrics/health endpoint startup.Source: Coding guidelines
🤖 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 `@crates/scout/src/stream.rs`:
- Around line 118-120: Update the client.scout_stream call in the
response-stream setup to record ScoutStreamConnection { outcome: Outcome::Error
} before propagating any handshake failure. Preserve successful stream
initialization and the existing error propagation behavior, while ensuring both
create_forge_client and scout_stream failures increment the connection error
metric.
---
Nitpick comments:
In `@crates/scout/src/cfg/command_line.rs`:
- Around line 111-117: The metrics listen address is validated too late as a
string. In crates/scout/src/cfg/command_line.rs:111-117, change CommandLine’s
metrics_listen_addr field to Option<SocketAddr> and ensure the SocketAddr type
is imported; in crates/scout/src/main.rs:179-182, remove the manual
addr_str.parse() and custom invalid-address error handling, passing
config.metrics_listen_addr directly to the metrics/health endpoint startup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a92e7ba3-9717-421a-8964-79859ad1ab41
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/scout/Cargo.tomlcrates/scout/src/cfg/command_line.rscrates/scout/src/main.rscrates/scout/src/metrics.rscrates/scout/src/stream.rs
scout -- the on-host control plane -- was invisible to Prometheus: no metrics and no /metrics endpoint, only logs. This stands one up through the shared metrics-endpoint crate and counts the control loop's work.
- With `--metrics-listen-addr` set, scout installs the framework meter, registers `carbide_log_events_total` (so the existing error/warn sites gain an error-rate signal for free), and serves `/metrics`. Without the flag scout stays a pure client as before -- the endpoint is opt-in, and the counters record nothing until it is configured.
- `carbide_scout_actions_total{action, outcome}` counts each control-loop action by kind and result; `carbide_scout_stream_connections_total{outcome}` and `carbide_scout_stream_reconnects_total` track the ScoutStream lifecycle.
- A failed log-error action now propagates instead of being swallowed, so its outcome records as an error rather than a silent success.
- The existing `error!`/`warn!` lines stay as logs -- their rate rides on `carbide_log_events_total`, so there are no per-site metric siblings.
scout's /metrics is binary-local (not scraped by `test_integration`), so there is no catalogue change.
This supports NVIDIA#3173
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3173-scout
branch
from
68f3326 to
e2ee040
Compare
Contributor
Author
|
Thanks @coderabbitai -- both addressed:
|
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 3539 --repo NVIDIA/infra-controller --json title,body,commits,files -q '.files[].path'
echo "---diff stream.rs---"
gh pr diff 3539 --repo NVIDIA/infra-controller -- crates/scout/src/stream.rs
echo "---diff command_line.rs---"
gh pr diff 3539 --repo NVIDIA/infra-controller -- crates/scout/src/cfg/command_line.rs
echo "---diff main.rs---"
gh pr diff 3539 --repo NVIDIA/infra-controller -- crates/scout/src/main.rsLength of output: 1074 🏁 Script executed: #!/bin/bash
gh pr diff 3539 --repo NVIDIA/infra-controller > /tmp/pr3539.diff
echo "---stream.rs---"
awk '/^diff --git a\/crates\/scout\/src\/stream.rs/,/^diff --git a\/crates\/scout\/src\/[^s]/' /tmp/pr3539.diff | sed '$d'
echo "---command_line.rs---"
awk '/^diff --git a\/crates\/scout\/src\/cfg\/command_line.rs/,0' /tmp/pr3539.diff | sed -n '1,80p'
echo "---main.rs relevant---"
grep -n "metrics_listen_addr\|SocketAddr" /tmp/pr3539.diffLength of output: 6310
Both look solid. I'll kick off the full review now. ᕮ(•ᴥ•)ᕭ ~ happy to see this landed cleanly ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/scout/src/stream.rs (1)
92-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication in the emit-then-map-error pattern.
The
emit(ScoutStreamConnection{outcome: Outcome::Error}); ScoutStreamError::X(...)sequence is repeated verbatim across both error paths. A tiny helper (e.g., a closure factory or arecord_connection_errorfn) could collapse this, though with only two call sites the current inline form is still perfectly readable.♻️ Optional consolidation
+fn record_connection_error<E: Into<ScoutStreamError>>(e: E) -> ScoutStreamError { + emit(ScoutStreamConnection { + outcome: Outcome::Error, + }); + e.into() +} + async fn run_scout_stream_loop( machine_id: MachineId, options: &Options, ) -> Result<(), ScoutStreamError> { - let mut client = client::create_forge_client(options).await.map_err(|e| { - emit(ScoutStreamConnection { - outcome: Outcome::Error, - }); - ScoutStreamError::ClientError(e.to_string()) - })?; + let mut client = client::create_forge_client(options) + .await + .map_err(|e| record_connection_error(ScoutStreamError::ClientError(e.to_string())))?;Also applies to: 119-128
🤖 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 `@crates/scout/src/stream.rs` around lines 92 - 97, Optionally consolidate the repeated connection-error handling in the client creation and subsequent error paths around create_forge_client and the second map_err block by introducing a small shared helper or closure that emits ScoutStreamConnection with Outcome::Error and constructs the corresponding ScoutStreamError. Replace both inline emit-then-map-error sequences while preserving each path’s existing error variant and message.
🤖 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.
Nitpick comments:
In `@crates/scout/src/stream.rs`:
- Around line 92-97: Optionally consolidate the repeated connection-error
handling in the client creation and subsequent error paths around
create_forge_client and the second map_err block by introducing a small shared
helper or closure that emits ScoutStreamConnection with Outcome::Error and
constructs the corresponding ScoutStreamError. Replace both inline
emit-then-map-error sequences while preserving each path’s existing error
variant and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 698b164e-26db-4189-84f7-334ae9237572
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/scout/Cargo.tomlcrates/scout/src/cfg/command_line.rscrates/scout/src/main.rscrates/scout/src/metrics.rscrates/scout/src/stream.rs
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Nothing further needed from my side here. ᕮ(•ᴥ•)ᕭ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/scout/src/metrics.rs (1)
77-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment undersells the
erroroutcome's coverage.The comment says
errorfires only "when the client could not be built," butstream.rsnow also emitsOutcome::Erroronclient.scout_stream(...)handshake failures (a separate, later failure point). Worth updating the doc so operators reading this metric's semantics aren't misled about which failures are counted.✏️ Proposed doc fix
-/// A scout stream connection attempt resolved -- `ok` once the bidirectional -/// stream is established, `error` when the client could not be built. +/// A scout stream connection attempt resolved -- `ok` once the bidirectional +/// stream is established, `error` when the client could not be built or the +/// stream handshake with the API failed.🤖 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 `@crates/scout/src/metrics.rs` around lines 77 - 90, Update the doc comment for ScoutStreamConnection to state that the error outcome covers both client construction failures and scout_stream handshake failures. Keep the existing description of successful bidirectional stream establishment and the metric definition unchanged.
🤖 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.
Nitpick comments:
In `@crates/scout/src/metrics.rs`:
- Around line 77-90: Update the doc comment for ScoutStreamConnection to state
that the error outcome covers both client construction failures and scout_stream
handshake failures. Keep the existing description of successful bidirectional
stream establishment and the metric definition unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a0e4695-445e-48e3-8057-4a2354f7b72a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/scout/Cargo.tomlcrates/scout/src/cfg/command_line.rscrates/scout/src/main.rscrates/scout/src/metrics.rscrates/scout/src/stream.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
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.
scout -- the on-host control plane -- was invisible to Prometheus: no metrics and no /metrics endpoint, only logs. This stands one up through the shared metrics-endpoint crate and counts the control loop's work.
--metrics-listen-addrset, scout installs the framework meter, registerscarbide_log_events_total(so the existing error/warn sites gain an error-rate signal for free), and serves/metrics. Without the flag scout stays a pure client as before -- the endpoint is opt-in, and the counters record nothing until it is configured.carbide_scout_actions_total{action, outcome}counts each control-loop action by kind and result;carbide_scout_stream_connections_total{outcome}andcarbide_scout_stream_reconnects_totaltrack the ScoutStream lifecycle.error!/warn!lines stay as logs -- their rate rides oncarbide_log_events_total, so there are no per-site metric siblings.scout's /metrics is binary-local (not scraped by
test_integration), so there is no catalogue change.This supports #3173