Add metrics for config-load failures and startup latency (#303) - #379
Open
leynos wants to merge 6 commits into
Open
Add metrics for config-load failures and startup latency (#303)#379leynos wants to merge 6 commits into
leynos wants to merge 6 commits into
Conversation
Contributor
|
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:
SummaryAdd configuration-load observability for startup configuration resolution.
Addresses issue WalkthroughStartup configuration resolution and merging now record success or failure outcomes and elapsed duration. Tests verify the emitted metrics. The developer guide documents metric names, labels, buckets, and recorder behaviour. ChangesConfiguration-load observability
Sequence Diagram(s)sequenceDiagram
participant Startup as Startup
participant Config as Configuration loading
participant Metrics as Metrics façade
Startup->>Config: Resolve and merge configuration
Config-->>Startup: Return success or early failure
Startup->>Metrics: Record outcome counter
Startup->>Metrics: Record elapsed duration histogram
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings, 5 inconclusive)
✅ Passed checks (11 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideAdds config-load observability by instrumenting startup configuration resolution with metrics, refactoring error handling, and documenting the new metrics, along with tests using a debugging recorder. Sequence diagram for configuration-load metrics and error handlingsequenceDiagram
participant main
participant resolve_configuration
participant cli as cli_merge
participant metrics_facade
participant handle_config_load_error
main->>resolve_configuration: resolve_configuration(parsed_cli, matches)
resolve_configuration->>cli: cli::resolve_merged_diag_json(parsed_cli, matches)
resolve_configuration-->>resolve_configuration: DiagMode::from_json_enabled(...)
resolve_configuration->>cli: cli::merge_with_config(parsed_cli, matches)
resolve_configuration-->>metrics_facade: record_config_load_metrics(elapsed, merged.is_ok())
metrics_facade-->>metrics_facade: metrics::histogram!(CONFIG_LOAD_DURATION_SECONDS)
metrics_facade-->>metrics_facade: metrics::counter!(CONFIG_LOAD_TOTAL)
resolve_configuration-->>main: (mode, merged)
alt [merge succeeded]
main-->>main: merged.with_default_command()
main-->>main: configure_runtime(...)
else [merge failed]
main->>handle_config_load_error: handle_config_load_error(err, mode)
handle_config_load_error-->>main: ExitCode::FAILURE
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
Configuration-load errors surface explicitly via `Result` boundaries and structured logging, but no metrics existed to detect production trends (failure rates, startup latency). Introduce a config-load observability boundary in `src/main.rs`: `resolve_configuration` spans diagnostic-mode resolution through the layer merge and calls `record_config_load_metrics`, which emits a `netsuke_config_load_total` counter labelled by `outcome` (`success`/`failure`) and a `netsuke_config_load_duration_seconds` histogram. The merge error path is extracted into `handle_config_load_error`. Recording goes through the `metrics` façade, so the instruments are no-ops unless an operator installs a recorder; Netsuke emits the measurements without bundling an exporter. Document the counter names, label conventions, and histogram buckets in `docs/developers-guide.md`. Add unit tests (using `metrics-util`'s debugging recorder) asserting the counter's outcome label and the single duration sample for both success and failure.
lodyai
Bot
force-pushed
the
issue-303-config-load-metrics
branch
from
August 9, 2026 01:22
400b733 to
700cdd8
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
August 9, 2026 02:32
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 700cdd8e74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+279
to
+280
| metrics::histogram!(CONFIG_LOAD_DURATION_SECONDS).record(elapsed.as_secs_f64()); | ||
| metrics::counter!(CONFIG_LOAD_TOTAL, "outcome" => outcome).increment(1); |
There was a problem hiding this comment.
For every normal invocation of the standalone netsuke binary, these calls are discarded by the metrics façade because neither main nor any other production path installs a recorder (confirmed by a repository-wide search for recorder installation). An operator cannot install one from outside an already compiled executable, so the new counter and histogram are unavailable in production despite the documented observability contract; initialize an exporter early in application startup or expose a real recorder configuration path.
AGENTS.md reference: AGENTS.md:L376-L378
Useful? React with 👍 / 👎.
![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: 3
🤖 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 `@docs/developers-guide.md`:
- Around line 2570-2577: Update the netsuke_config_load_duration_seconds
documentation to state that it records one sample per startup that reaches
configuration resolution, matching the timing and record_config_load_metrics
behavior in main. Do not claim coverage of every startup attempt unless the
timer is moved before parse_cli_or_exit and parse failures are recorded.
In `@src/main_tests.rs`:
- Around line 45-66: Strengthen the metric assertions in the test around
CONFIG_LOAD_TOTAL and CONFIG_LOAD_DURATION_SECONDS: require the counter to
contain exactly one label and that label to be outcome=expected_outcome, then
assert the sole histogram sample equals Duration::from_millis(1).as_secs_f64().
Preserve the existing count and single-sample checks.
- Around line 41-43: Expand the tests around run_with_args to capture metrics
for the successfully merged configuration, the resolve_json_mode_or_exit failure
path, and the merge_cli_or_exit failure path. Replace or supplement the direct
record_config_load_metrics test so each configuration path verifies its
timer/metric behavior, including the call sites associated with lines 103, 114,
and 118.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d920bda5-ce1b-40f9-bedb-5fe1ed469e7f
📒 Files selected for processing (3)
docs/developers-guide.mdsrc/main.rssrc/main_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
Comment on lines
+2570
to
+2577
| - `netsuke_config_load_total` — a counter incremented once per startup | ||
| configuration-load attempt. It carries a single label `outcome` with values | ||
| `success` or `failure`, where `failure` corresponds to diagnostic-mode | ||
| resolution or a `merge_with_config` error. Use it to compute the | ||
| configuration-load failure rate. | ||
| - `netsuke_config_load_duration_seconds` — a histogram recording the | ||
| wall-clock duration of the configuration-load phase in seconds (one sample | ||
| per startup). Suggested operator bucket boundaries: `0.001, 0.005, 0.01, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Scope the “one sample per startup” claim to the measured phase.
Change the wording to “one sample per startup that reaches configuration resolution”, or start the timer before parse_cli_or_exit if the metric must cover every startup attempt. The supplied src/main.rs path starts timing only after CLI parsing succeeds, and the parse-error branch returns without calling record_config_load_metrics. The current wording can cause operators to under-count startup failures.
Based on the supplied src/main.rs context.
🤖 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 `@docs/developers-guide.md` around lines 2570 - 2577, Update the
netsuke_config_load_duration_seconds documentation to state that it records one
sample per startup that reaches configuration resolution, matching the timing
and record_config_load_metrics behavior in main. Do not claim coverage of every
startup attempt unless the timer is moved before parse_cli_or_exit and parse
failures are recorded.
Comment on lines
+41
to
+43
| let metrics = captured_metrics(|| { | ||
| record_config_load_metrics(Duration::from_millis(1), succeeded); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/main_tests.rs --items all --type function
rg -n -C 8 --type rust \
'\b(run_with_args|resolve_json_mode_or_exit|merge_cli_or_exit|record_config_load_metrics|captured_metrics)\b' \
src/main.rs src/main_tests.rsRepository: leynos/netsuke
Length of output: 10156
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- src/main_tests.rs ---'
sed -n '1,90p' src/main_tests.rs
printf '%s\n' '--- src/main.rs startup path ---'
sed -n '70,130p' src/main.rs
printf '%s\n' '--- configuration and startup tests ---'
rg -n -C 6 --type rust \
'run_with_args|merge_with_config|resolve_merged_json|configuration.*fail|config.*fail|ConfigError|ExitCode' \
src tests
printf '%s\n' '--- test layout ---'
find tests -maxdepth 2 -type f -print 2>/dev/null | sortRepository: leynos/netsuke
Length of output: 50370
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- startup integration-test helpers ---'
sed -n '1,145p' tests/startup_diagnostics_tests.rs
printf '%s\n' '--- config-driven merge-failure tests ---'
sed -n '190,275p' tests/logging_stderr/json.rs
sed -n '330,380p' tests/advanced_usage_tests.rs
printf '%s\n' '--- CLI command and config defaults ---'
rg -n -C 5 --type rust \
'enum Commands|struct Cli|default_value|command\(' \
src/cli/parser.rs src/cli/config.rs src/runner.rs
printf '%s\n' '--- metric recorder and run_with_args references ---'
rg -n --type rust \
'with_local_recorder|CONFIG_LOAD_TOTAL|CONFIG_LOAD_DURATION_SECONDS|run_with_args' \
src testsRepository: leynos/netsuke
Length of output: 18228
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
main = Path("src/main.rs").read_text()
tests = Path("src/main_tests.rs").read_text()
body = re.search(
r"fn run_with_args\(.*?\n\}\n\n/// Handle for adjusting",
main,
re.S,
).group(0)
print("run_with_args metric calls:")
for line_no, line in enumerate(body.splitlines(), 1):
if "record_config_load_metrics" in line:
print(f" {line_no}: {line.strip()}")
print("run_with_args configuration-path calls:")
for name in ("parse_cli_or_exit", "resolve_json_mode_or_exit", "merge_cli_or_exit"):
print(f" {name}: {len(re.findall(r'\\b' + name + r'\\b', body))}")
print("main_tests run_with_args references:")
for line_no, line in enumerate(tests.splitlines(), 1):
if "run_with_args" in line:
print(f" {line_no}: {line.strip()}")
print("direct metric-helper test references:")
for line_no, line in enumerate(tests.splitlines(), 1):
if "record_config_load_metrics" in line:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- all metric references ---'
rg -n --type rust \
'with_local_recorder|CONFIG_LOAD_TOTAL|CONFIG_LOAD_DURATION_SECONDS|record_config_load_metrics|run_with_args' \
src tests || trueRepository: leynos/netsuke
Length of output: 387
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
main = Path("src/main.rs").read_text()
tests = Path("src/main_tests.rs").read_text()
match = re.search(
r"fn run_with_args\(.*?\n\}\n\n/// Handle for adjusting",
main,
re.S,
)
if not match:
raise SystemExit("run_with_args body not found")
body = match.group(0)
print("run_with_args metric calls:")
for line_no, line in enumerate(body.splitlines(), 1):
if "record_config_load_metrics" in line:
print(f" {line_no}: {line.strip()}")
print("run_with_args configuration-path calls:")
for name in ("parse_cli_or_exit", "resolve_json_mode_or_exit", "merge_cli_or_exit"):
count = len(re.findall(r"\b" + name + r"\b", body))
print(f" {name}: {count}")
print("main_tests run_with_args references:")
for line_no, line in enumerate(tests.splitlines(), 1):
if "run_with_args" in line:
print(f" {line_no}: {line.strip()}")
print("direct metric-helper test references:")
for line_no, line in enumerate(tests.splitlines(), 1):
if "record_config_load_metrics" in line:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- all metric references ---'
rg -n --type rust \
'with_local_recorder|CONFIG_LOAD_TOTAL|CONFIG_LOAD_DURATION_SECONDS|record_config_load_metrics|run_with_args' \
src tests || trueRepository: leynos/netsuke
Length of output: 2705
Exercise run_with_args across all configuration paths.
Capture metrics while testing a successfully merged configuration, a resolve_json_mode_or_exit failure, and a merge_cli_or_exit failure. The direct record_config_load_metrics test cannot detect a missing timer or call at lines 103, 114, or 118.
🤖 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/main_tests.rs` around lines 41 - 43, Expand the tests around
run_with_args to capture metrics for the successfully merged configuration, the
resolve_json_mode_or_exit failure path, and the merge_cli_or_exit failure path.
Replace or supplement the direct record_config_load_metrics test so each
configuration path verifies its timer/metric behavior, including the call sites
associated with lines 103, 114, and 118.
Source: Coding guidelines
Comment on lines
+45
to
+66
| let counter = metrics | ||
| .iter() | ||
| .find(|(name, _, _)| name == CONFIG_LOAD_TOTAL) | ||
| .expect("config-load counter should be recorded"); | ||
| assert!( | ||
| counter | ||
| .1 | ||
| .iter() | ||
| .any(|label| { label.key() == "outcome" && label.value() == expected_outcome }), | ||
| "counter should carry outcome={expected_outcome}: {:?}", | ||
| counter.1 | ||
| ); | ||
| assert_eq!(counter.2, DebugValue::Counter(1)); | ||
|
|
||
| let histogram = metrics | ||
| .iter() | ||
| .find(|(name, _, _)| name == CONFIG_LOAD_DURATION_SECONDS) | ||
| .expect("config-load duration histogram should be recorded"); | ||
| let DebugValue::Histogram(samples) = &histogram.2 else { | ||
| panic!("expected a histogram value, got {:?}", histogram.2); | ||
| }; | ||
| assert_eq!(samples.len(), 1, "exactly one duration sample expected"); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete metric contract.
Assert that the counter has exactly one outcome label. Assert that the
histogram sample equals Duration::from_millis(1).as_secs_f64(). The current
test passes if the metric records zero, uses another unit, or adds labels.
Proposed test assertions
assert!(
counter
.1
.iter()
.any(|label| { label.key() == "outcome" && label.value() == expected_outcome }),
"counter should carry outcome={expected_outcome}: {:?}",
counter.1
);
+ assert_eq!(counter.1.len(), 1, "counter must have only the outcome label");
assert_eq!(counter.2, DebugValue::Counter(1));
let histogram = metrics
.iter()
.find(|(name, _, _)| name == CONFIG_LOAD_DURATION_SECONDS)
.expect("config-load duration histogram should be recorded");
let DebugValue::Histogram(samples) = &histogram.2 else {
panic!("expected a histogram value, got {:?}", histogram.2);
};
- assert_eq!(samples.len(), 1, "exactly one duration sample expected");
+ assert_eq!(samples.as_slice(), &[Duration::from_millis(1).as_secs_f64()]);As per coding guidelines, validate new features with unit tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let counter = metrics | |
| .iter() | |
| .find(|(name, _, _)| name == CONFIG_LOAD_TOTAL) | |
| .expect("config-load counter should be recorded"); | |
| assert!( | |
| counter | |
| .1 | |
| .iter() | |
| .any(|label| { label.key() == "outcome" && label.value() == expected_outcome }), | |
| "counter should carry outcome={expected_outcome}: {:?}", | |
| counter.1 | |
| ); | |
| assert_eq!(counter.2, DebugValue::Counter(1)); | |
| let histogram = metrics | |
| .iter() | |
| .find(|(name, _, _)| name == CONFIG_LOAD_DURATION_SECONDS) | |
| .expect("config-load duration histogram should be recorded"); | |
| let DebugValue::Histogram(samples) = &histogram.2 else { | |
| panic!("expected a histogram value, got {:?}", histogram.2); | |
| }; | |
| assert_eq!(samples.len(), 1, "exactly one duration sample expected"); | |
| let counter = metrics | |
| .iter() | |
| .find(|(name, _, _)| name == CONFIG_LOAD_TOTAL) | |
| .expect("config-load counter should be recorded"); | |
| assert!( | |
| counter | |
| .1 | |
| .iter() | |
| .any(|label| { label.key() == "outcome" && label.value() == expected_outcome }), | |
| "counter should carry outcome={expected_outcome}: {:?}", | |
| counter.1 | |
| ); | |
| assert_eq!(counter.1.len(), 1, "counter must have only the outcome label"); | |
| assert_eq!(counter.2, DebugValue::Counter(1)); | |
| let histogram = metrics | |
| .iter() | |
| .find(|(name, _, _)| name == CONFIG_LOAD_DURATION_SECONDS) | |
| .expect("config-load duration histogram should be recorded"); | |
| let DebugValue::Histogram(samples) = &histogram.2 else { | |
| panic!("expected a histogram value, got {:?}", histogram.2); | |
| }; | |
| assert_eq!(samples.as_slice(), &[Duration::from_millis(1).as_secs_f64()]); |
🤖 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/main_tests.rs` around lines 45 - 66, Strengthen the metric assertions in
the test around CONFIG_LOAD_TOTAL and CONFIG_LOAD_DURATION_SECONDS: require the
counter to contain exactly one label and that label to be
outcome=expected_outcome, then assert the sole histogram sample equals
Duration::from_millis(1).as_secs_f64(). Preserve the existing count and
single-sample checks.
Source: Coding guidelines
Document that `netsuke_config_load_duration_seconds` records one sample for startups that reach configuration resolution, matching the timer's placement after CLI parsing.
Describe `NETSUKE_METRICS_LISTEN` for operators, including its socket syntax, startup timing, process lifetime, default-disabled behaviour, and startup failures for invalid or unavailable addresses.
Precede the `NETSUKE_METRICS_LISTEN` shell example with its required `tested-example` marker so documentation example validation can load it.
Install an opt-in Prometheus recorder before configuration resolution so operators can scrape the startup metrics from the standalone binary. Cover each configuration path through the startup orchestration and keep the metric contract assertions exact where timing is deterministic.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Describe the optional Prometheus recorder and configuration-load metrics at the application entry-point boundary.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
Closes #303
Adds the metrics instrumentation requested as a follow-up to PR #297: a config-load failure counter and a startup-latency histogram, plus developer documentation.
Changes
Cargo.toml: add themetricsfaçade (runtime) andmetrics-util(dev, debugging feature).src/main.rs: introduceresolve_configuration(spanscli::resolve_merged_diag_jsonthroughcli::merge_with_config) andrecord_config_load_metrics, emitting:netsuke_config_load_total— counter labelledoutcome(success/failure);netsuke_config_load_duration_seconds— duration histogram.The merge error path is extracted into
handle_config_load_error. Becausemetricsis a façade, the instruments are no-ops until an operator installs a recorder; Netsuke bundles none.docs/developers-guide.md: new Configuration-load observability subsection documenting counter names, label conventions, and suggested histogram buckets.Testing
metrics_util::debugging::DebuggingRecorder+metrics::with_local_recorderto assert the counter carriesoutcome=failure/outcome=successand that the histogram records exactly one sample.Structured log fields (
operation,error_category) and per-phase counter labels are the scope of the follow-up #304.Validation
make check-fmt/make markdownlint/make lint/make test— pass (37 suites)🤖 Generated with Claude Code
Summary by Sourcery
Instrument startup configuration loading with metrics and document their usage.
New Features:
Enhancements:
Build:
Documentation:
Tests:
References