Repository navigation
feat(otel): a span on the ten error-returning entry points the instrumentation scan found - #2763
Conversation
…mentation scan found minimal#2449 (advisory): the graph loader (`load`, `load_idx`, `load_toplevels`), the sandbox's start gate and rootfs setup (`hold_start`, `new`), the diagnostics bundle's process table (`process_tree`, `all_processes`, `proc_table_text`) and the graph wire writer (`write_record`, `write_graph`) returned errors with no span of their own, so a name that does not resolve, a start gate that wedges a launch, or a frame that fails to write had nothing in the trace to land on. Each gets `#[tracing::instrument]` with `err`, in the pattern `graph/planner.rs` already uses: the entry points at info, the per-item helpers at debug, the per-frame writer at trace, all `skip_all` (a payload is not a span field). diagnostics gains the tracing dependency it lacked. Closes #2449 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Instrument process diagnostics crates/diagnostics/Cargo.toml, crates/diagnostics/src/procs.rs |
The diagnostics crate adds the workspace tracing dependency. Instrumentation was added to process_tree, all_processes, and Linux proc_table_text. |
Instrument graph loading and writing crates/graph/src/loader.rs, crates/graph/src/wire.rs |
Instrumentation was added to Loader::load, Loader::load_idx, Loader::load_toplevels, write_record, and write_graph. The annotations use debug, info, or trace level and record errors. |
Instrument sandbox setup crates/sandbox/src/lib.rs |
Info-level instrumentation was added to Sandbox::hold_start and Sandbox::new. Both annotations record errors. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature · Severity of issue fixed: Low
Suggested reviewers: norrietaylor
Merge Risk: 
b1e98
-
No actionable merge-blocking risk remains from this instrumentation-only change.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the primary change: adding OpenTelemetry spans to ten error-returning entry points. It is specific and related to the changeset, although somewhat long. Description check The description provides a detailed summary, identifies the linked issue, and reports testing results. It does not include the template's explicit Checklist section, but the missing checklist is non-c… Linked Issues check The pull request addresses all ten coding objectives in directly linked issue #2449. It adds#[tracing::instrument]witherrandskip_alltoLoader::load,load_idx,load_toplevels, `Sandbo…Out of Scope Changes check The reviewed changes are limited to the ten functions named by issue #2449and the requiredtracingdependency incrates/diagnostics/Cargo.toml. The summaries report no behavior or public API chan…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Description check
-
Explanation
The description provides a detailed summary, identifies the linked issue, and reports testing results. It does not include the template's explicit Checklist section, but the missing checklist is non-critical because the required summary and testing information are present.
Full details: Linked Issues check
-
Explanation
The pull request addresses all ten coding objectives in directly linked issue
#2449. It adds#[tracing::instrument]witherrandskip_alltoLoader::load,load_idx,load_toplevels,Sandbox::hold_start,Sandbox::new,process_tree,all_processes,proc_table_text,write_record, andwrite_graph. The reported levels match the issue: loader helpers usedebug, entry points useinfo, andwrite_recordusestrace. The diagnostics crate also adds its requiredtracingdependency.
Full details: Out of Scope Changes check
-
Explanation
The reviewed changes are limited to the ten functions named by issue
#2449and the requiredtracingdependency incrates/diagnostics/Cargo.toml. The summaries report no behavior or public API changes. No unrelated implementation change is identified.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 4 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit hops beside the logs,
New spans mark paths through fields and fog.
Errors leave a trace behind,
While quiet arguments stay confined.
The burrow hums with signals bright.
Comment @coderabbitai help to get the list of available commands.
|
Approved: no findings at Nothing to do. Clobber: none seen. |
There was a problem hiding this comment.
Every new span uses skip_all, and none of the recorded errors carry anything sensitive. CI is green.
Non-blocking (low):
graph/src/loader.rs:45,59,84:err(Debug)on the mutually recursiveload/load_idx/load_toplevelsemits an ERROR at every level of the recursion. One missing upstream package makesmip buildprint about fiveERROR graph::loader: error=NoSuchPkg {..}lines ahead of its own error, more for deeper chains, since mip's stderr filter isinfo. Keeperronly onload_toplevels, or useerr(level = "debug", Debug)on the inner two.graph/src/wire.rs:248,266: these spans are on the syncGraphWriter, which only tests and fuzzing reach. The production path,AsyncGraphWriter(wire.rs:748-769, used byremote-client/src/lib.rs:94,286), still has no span.diagnostics/src/procs.rs:103,149,180:collect_step!already records these failures. Witherradded, minimald reports a/procfailure three times, andmin bugnow prints an ERROR on the terminal for a hostprocess_treefailure that used to be silent.- Nit:
diagnostics/Cargo.toml:11putstracingaboveanyhow, out of alphabetical order.
Separately, the PR body says this follows a pattern in graph/planner.rs, but planner.rs:296,305 use a bare #[tracing::instrument] with no skip_all and no err.
minimal#2449 (advisory): the graph loader (
load,load_idx,load_toplevels), the sandbox'sstart gate and rootfs setup (
hold_start,new), the diagnostics bundle's process table(
process_tree,all_processes,proc_table_text) and the graph wire writer (write_record,write_graph) returned errors with no span of their own, so a name that does not resolve, a startgate that wedges a launch, or a frame that fails to write had nothing in the trace to land on.
Each gets
#[tracing::instrument]witherr, in the patterngraph/planner.rsalready uses: theentry points at info, the per-item helpers at debug, the per-frame writer at trace, all
skip_all(a payload is not a span field). diagnostics gains the tracing dependency it lacked.
Closes #2449
Clippy clean; graph, sandbox and diagnostics tests pass on anvil (the one failure there is the box's known root-capabilities integration test). Prepush mirror: job 01478.
🤖 Generated with Claude Code
Summary by CodeRabbit