Add metrics for config-load failures and startup latency (#303) - #379
Merged
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:
Summary
WalkthroughConfiguration loading now uses cached discovery outcomes, injected environment and clock providers, centralised startup orchestration, bounded diagnostics, and phase plus startup metrics. Tests, benchmarks, and documentation cover the new flow. ChangesConfiguration loading
Sequence Diagram(s)sequenceDiagram
participant Application
participant ConfigurationLoadContext
participant DiscoveryOutcome
participant ConfigurationMerge
participant MetricsRecorder
Application->>ConfigurationLoadContext: load configuration with injected environment and clock
ConfigurationLoadContext->>DiscoveryOutcome: resolve JSON mode and retain layers
ConfigurationLoadContext->>ConfigurationMerge: merge cached layers with CLI values
ConfigurationLoadContext->>MetricsRecorder: record phase and startup results
ConfigurationLoadContext-->>Application: return merged CLI or exit code
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, 1 warning, 4 inconclusive)
✅ Passed checks (13 passed)
✨ 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}
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
Heads-up: issue #303 has been closed as subsumed by #304, which is now the single tracking issue for config-load observability. #547 is designated the vehicle since it adds |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
issue-303-config-load-metrics
branch
from
August 14, 2026 00:47
0f68423 to
20b8841
Compare
leynos
changed the base branch from
main
to
issue-304-improve-observability-of-config-load-error-paths-structured-log-fields-metrics-by-phase
August 14, 2026 00:47
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Include diagnostic resolution in its phase duration while retaining cached diagnostics for both success and failure. Keep configuration orchestration in its focused module and require non-empty histogram samples in verbose tests.
Document the bounded tracing fields and make clear that configuration file names and full paths are never exposed.
Reduce string-heavy helper arguments while preserving the configuration metrics, precedence cases, and child-process behaviour under test.
Encapsulate captured diagnostics, failure fields, and metrics records in test-domain types. Separate metrics snapshot assertions from tracing scenarios so both test modules remain below the enforced size limit.
Keep JSON discovery side-effect free and carry deferred diagnostics to the startup composition boundary. Reuse the first discovery pass for JSON preference and merging without cloning complete file layers. Inject configuration environment access, add cache and tracing proofs, and benchmark the large cached path.
Align ADR-011 and the users guide with the current cached-layer resolution flow, and shorten the ADR-011 index label for readability.
Document the tuple returned by both resolver APIs and the diagnostic and layer extraction steps required before cached merging.
Restore the startup and diagnostic test bindings lost during the conflict replay, and keep configuration discovery on its shared cached layer helper. Remove superseded direct metric coverage and replay-only documentation whitespace so the rebased branch builds and validates.
Remove the public resolve_json_and_layers_with_env alias, a pure delegation to resolve_json_and_layers_outcome_with_env with an identical (OrthoResult<bool>, DiscoveryOutcome) signature, and standardize every caller, unit test, compile-pass fixture, benchmark, and documentation reference on the outcome variant. The alias had no production caller; keeping one resolver API removes a duplicate interface without distinct behaviour. Fix the review's documentation findings: shorten the ADR-013 index label, correct the OrthoConfig 0.9.0 reference, join the split users-guide sentence, and document the full metric contract (internal phase-level config_load_* series plus operator-facing netsuke_config_load_* startup-attempt series) in the migration guide. Co-Authored-By: Claude <noreply@anthropic.com>
Update the configuration-loading documentation to describe the cached discovery boundary, and keep integration-test fixture paths UTF-8 and capability-oriented until the existing process helpers need OS paths.
Defer discovery diagnostics from JSON-resolution queries and retain the bounded startup metrics in the process recorder. Expand the focused test module documentation and cover the accepted startup metric series.
Keep the recorder scenario readable while preserving its exact retained series and rejected-registration checks.
Move the mixed metric registration into a focused test helper while retaining each series and snapshot assertion unchanged.
leynos
force-pushed
the
issue-303-config-load-metrics
branch
from
August 21, 2026 17:02
68c09b2 to
4da4363
Compare
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.
Owner
Author
|
@coderabbitai Have the following now been resolved? docs/users-guide.md (1)
pass the cached layers to return shape. Both supplied functions return Proposed fix- `netsuke::cli::resolve_json_and_layers_with_env` function returns the resolved
- JSON boolean together with `DiscoveredLayers`; pass those layers to
- `netsuke::cli::merge_with_cached_file_layers` for the full merge.
+ `netsuke::cli::resolve_json_and_layers_with_env` function returns the resolved
+ JSON boolean together with a `DiscoveryOutcome`. Call `into_layers()` on that
+ outcome, then pass the result to `netsuke::cli::merge_with_cached_file_layers`
+ for the full merge.🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In tests/advanced_usage_tests.rs (1)
focused sibling module.
files and record a focused follow-up when an unrelated split is out of scope. As per coding guidelines, “Keep each Rust source file at 400 lines or fewer.” As per path instructions, “Files must not exceed 400 lines in length.” 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Sources: Coding guidelines, Path instructions, Learnings |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR metadata and check runs ---'
gh pr view 379 --repo leynos/netsuke \
--json number,state,headRefOid,updatedAt,baseRefName,url
sha="$(gh pr view 379 --repo leynos/netsuke --json headRefOid --jq .headRefOid)"
gh api "repos/leynos/netsuke/commits/${sha}/check-runs?per_page=100" --paginate \
--jq '.check_runs[] | [.name, .status, (.conclusion // "null"), .details_url] | `@tsv`' || true
printf '%s\n' '--- Relevant files ---'
fd -a -t f -E target \
'diag.rs|config_load.rs|merge.rs|discovery.*\.rs|config_load.*tests\.rs|config_load_cached_merge\.rs' \
src tests benches 2>/dev/null | sort
printf '%s\n' '--- Symbol outlines ---'
for file in src/cli/diag.rs src/config_load.rs src/cli/merge.rs src/cli/discovery.rs; do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '--- Resolver, diagnostic, environment, ownership, cache, and benchmark evidence ---'
rg -n -C 6 -P \
'\b(resolve_json_and_layers_with_env|resolve_json_and_layers_outcome_with_env|resolve_merged_json_with_env|emit_diagnostics|ConfigurationLoadContext|ConfigEnvProvider|ConfigStdEnvProvider|json_from_layers|into_value\(\)|clone\(\)\.into_value\(|merge_with_cached_file_layers|remove_file|cached.*layer|cache.*merge|bench-config-load)\b' \
src tests benches Makefile docs Cargo.toml || true
printf '%s\n' '--- Focused implementations ---'
for file in src/cli/diag.rs src/config_load.rs src/cli/merge.rs src/cli/discovery.rs; do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
sed -n '1,340p' "$file"
fi
done
printf '%s\n' '--- Current PR changes in relevant paths ---'
git diff --no-ext-diff origin/main...HEAD -- \
src/cli/diag.rs src/config_load.rs src/cli/merge.rs src/cli/discovery.rs \
src/config_load_context_env_tests.rs src/config_load_metrics_tests.rs \
benches/config_load_cached_merge.rs Makefile \
tests/ui/config_cached_discovery_embedder_pass.rs || trueLength of output: 157216
Mark all three checks as resolved at current PR head
No further work is required for these checks. Do not create a follow-up issue. The current head also has successful You are interacting with an AI system. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR metadata and check runs ---'
gh pr view 379 --repo leynos/netsuke --json number,state,headRefOid,updatedAt,url
head_sha="$(gh pr view 379 --repo leynos/netsuke --json headRefOid --jq .headRefOid)"
gh api "repos/leynos/netsuke/commits/${head_sha}/check-runs?per_page=100" --paginate \
--jq '.check_runs[] | [.name, .status, .conclusion, .details_url] | `@tsv`' || true
printf '%s\n' '--- Module documentation headers ---'
sed -n '1,110p' tests/advanced_usage/config_precedence.rs
sed -n '1,130p' tests/logging_stderr/config_tracing_metrics.rs
printf '%s\n' '--- Resolver API, diagnostics, and call sites ---'
ast-grep outline src/cli/diag.rs --items all || true
rg -n -C 6 -P \
'\b(resolve_merged_json_with_env|resolve_json_and_layers_outcome_with_env|emit_diagnostics|defers_load_diagnostics)\b' \
src/cli/diag.rs src/cli/mod.rs src tests benches || true
sed -n '1,260p' src/cli/diag.rs
printf '%s\n' '--- Recorder admission, production installation, and recorder tests ---'
ast-grep outline src/observability_recorder.rs --items all || true
ast-grep outline src/observability_recorder_tests.rs --items all || true
rg -n -C 8 -P \
'\b(ConfigMetricsRecorder|accepts_registration|STARTUP_CONFIG_LOAD|CONFIG_LOAD_(COUNTER|DURATION)|netsuke_config_load|with_local_recorder|install)\b' \
src/observability_recorder.rs src/observability_recorder_tests.rs src/observability.rs src/main.rs src/config_load.rs || true
sed -n '1,260p' src/observability_recorder.rs
sed -n '1,260p' src/observability_recorder_tests.rsLength of output: 82106
The current check runs also show success for Do not create a follow-up issue. No remaining work is required for these three findings. You are interacting with an AI system. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR head ---'
gh pr view 379 --repo leynos/netsuke --json headRefOid,updatedAt,state,url
printf '%s\n' '--- Resolver API definitions and re-exports ---'
ast-grep outline src/cli/diag.rs --items all || true
sed -n '1,180p' src/cli/diag.rs
sed -n '1,120p' src/cli/mod.rs
rg -n -C 3 -P '\bresolve_json_and_layers(?:_outcome)?_with_env\b|\bDiscoveryOutcome\b|\binto_layers\s*\(' \
src tests benches docs || true
printf '%s\n' '--- User-guide cached-resolution section ---'
nl -ba docs/users-guide.md | sed -n '825,875p'
rg -n -C 6 -P \
'\bresolve_json_and_layers(?:_outcome)?_with_env\b|\bDiscoveryOutcome\b|\bDiscoveredLayers\b|\binto_layers\s*\(|\bmerge_with_cached_file_layers\b' \
docs/users-guide.md || true
printf '%s\n' '--- Advanced-usage test module layout and line counts ---'
wc -l tests/advanced_usage_tests.rs
fd -a -t f . tests/advanced_usage | sort | xargs -r -n1 sh -c 'printf "%5s %s\n" "$(wc -l < "$0")" "$0"'
ast-grep outline tests/advanced_usage_tests.rs --items all || true
sed -n '1,80p' tests/advanced_usage_tests.rs
sed -n '360,490p' tests/advanced_usage_tests.rs
printf '%s\n' '--- References to configuration precedence helpers ---'
rg -n -C 4 -P \
'\b(ConfigPrecedenceCase|verbose_config_precedence|assert_config_metrics_snapshot|contains_metric_record|contains_non_empty_histogram_record)\b' \
tests/advanced_usage_tests.rs tests/advanced_usage 2>/dev/null || trueLength of output: 38118 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Current user-guide cached-resolution section ---'
awk 'NR >= 975 && NR <= 1020 { printf "%5d %s\n", NR, $0 }' docs/users-guide.md
printf '%s\n' '--- Current user-guide resolver references ---'
rg -n -C 5 -P \
'\bresolve_json_and_layers(?:_outcome)?_with_env\b|\bDiscoveryOutcome\b|\bDiscoveredLayers\b|\binto_layers\s*\(|\bmerge_with_cached_file_layers\b' \
docs/users-guide.md || true
printf '%s\n' '--- Advanced-usage test file size ---'
awk 'END { printf "tests/advanced_usage_tests.rs: %d lines\n", NR }' tests/advanced_usage_tests.rs
printf '%s\n' '--- Advanced-usage sibling modules and sizes ---'
fd -a -t f . tests/advanced_usage -x sh -c \
'awk -v file="$1" "END { printf \"%5d %s\\n\", NR, file }" "$1"' sh {} | sort -k2
printf '%s\n' '--- Advanced-usage crate module declarations ---'
sed -n '1,100p' tests/advanced_usage_tests.rs
printf '%s\n' '--- Former configuration-precedence region ---'
sed -n '350,490p' tests/advanced_usage_tests.rs
printf '%s\n' '--- Configuration-precedence ownership ---'
rg -n -C 4 -P \
'\b(ConfigPrecedenceCase|verbose_config_precedence|assert_config_metrics_snapshot|contains_metric_record|contains_non_empty_histogram_record)\b' \
tests/advanced_usage_tests.rs tests/advanced_usage 2>/dev/null || true
printf '%s\n' '--- Current relevant checks ---'
sha="$(gh pr view 379 --repo leynos/netsuke --json headRefOid --jq .headRefOid)"
gh api "repos/leynos/netsuke/commits/${sha}/check-runs?per_page=100" --paginate \
--jq '.check_runs[] | select(.name | test("build-test|CodeScene Code Health"; "i")) | [.name, .status, (.conclusion // "null")] | `@tsv`' || trueLength of output: 17448
Do not update
You are interacting with an AI system. |
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
4 participants
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 bounded outcome and latency metrics while preserving cached discovery, deterministic testing, and parseable diagnostics.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
References