Improve config-load observability (#304) - #547
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Reviewer's GuideAdds process-level observability around the two configuration-loading phases by introducing bounded metrics, error categorization, and structured logging, and wires this into the CLI composition root and developer documentation. Sequence diagram for configuration-load observability and metrics snapshotsequenceDiagram
participant Main
participant Observability
participant MetricsRecorder
participant Tracing
Main->>Tracing: init_tracing
Main->>Observability: init_metrics
Observability->>MetricsRecorder: DebuggingRecorder::install
Main->>Observability: record_config_load(DIAG_MODE_PHASE)
Observability->>MetricsRecorder: counter!(CONFIG_LOAD_COUNTER)
Observability->>MetricsRecorder: histogram!(CONFIG_LOAD_DURATION)
Main->>Observability: record_config_load(MERGE_PHASE)
Observability->>MetricsRecorder: counter!(CONFIG_LOAD_COUNTER)
Observability->>MetricsRecorder: histogram!(CONFIG_LOAD_DURATION)
Main->>Observability: classify_error
Main->>Tracing: tracing::error
Main->>Observability: emit_metrics_snapshot
Observability->>MetricsRecorder: Snapshotter::snapshot
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Complex Methodsrc/observability.rs: tests.records_each_config_load_phase_and_outcome What lead to degradation?tests.records_each_config_load_phase_and_outcome has a cyclomatic complexity of 15, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Rust language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
This comment was marked as resolved.
This comment was marked as resolved.
Record bounded configuration-load outcomes and durations at the CLI boundary, and include the failing startup operation and error category in human-readable error logs. Install the application-owned debugging recorder so verbose runs emit a shutdown snapshot without affecting isolated tests.
Define the stable configuration-load metrics, structured log fields, recorder lifecycle, and raw-sample histogram policy so future changes preserve the operator-facing contract.
Satisfy the module-level test documentation contract enforced by Whitaker so the configuration observability suite remains lint-clean.
e659ee1 to
3c73c99
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.
Extract snapshot predicates from the configuration-load metric test so each expected record remains explicit while the test scenario stays straightforward to read.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: 37432ada78
ℹ️ 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".
| // The buffer was settled inside, before the branch that exits. | ||
| Err(code) => return code, | ||
| }; | ||
| let verbose = parsed_cli.verbose; |
There was a problem hiding this comment.
When verbose mode is enabled through a configuration file or NETSUKE_VERBOSE, parsed_cli.verbose remains the pre-merge default while merged_cli.verbose becomes true. Capturing the former here and passing it to the final finish_run therefore suppresses the metrics snapshot even though the command otherwise runs in verbose mode; use the merged value after a successful merge, retaining the pre-merge value only for earlier failure paths.
Useful? React with 👍 / 👎.
1 participant
Summary
This branch instruments the two configuration-loading phases so operators can
identify failures, compare outcomes, and inspect startup latency without
unbounded telemetry labels.
Closes #304.
Review walkthrough
Validation
make check-fmt: passedmake typecheck: passedmake lint: passedmake test: passed (1,913 nextest tests and doctests)make markdownlint: passedmake nixie: passedcoderabbit review --agent: passed with zero findings after each milestoneReferences
Summary by Sourcery
Instrument configuration loading phases with bounded metrics, structured error logging, and a process-wide metrics recorder to improve observability of config-load behavior and failures.
New Features:
Enhancements:
Documentation:
Tests: