Add -Zerror-metrics=PATH to save diagnostic metadata to disk
#119355
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
error_metrics
branch
from
3dfb6d3 to
ef75451
Compare
Add nightly only flag to store information about encountered errors to disk. The information being tracked is: - Error code - Path to error being emitted - Number of subdiagnostics - Number of structured suggestions
estebank
force-pushed
the
error_metrics
branch
from
ef75451 to
2ab3cb9
Compare
| if let Some(ref mut file) = &mut self.metrics { | ||
| let _ = writeln!( | ||
| file, | ||
| "{:?},{:?},{},{}", |
There was a problem hiding this comment.
Why are we using this custom format when we already have JSON diagnostic output?
Even if we shouldn't be emitting as much info as JSON-formatted diagnostics, we should perhaps make it easier for arbitrary programs to consume by formatting it like JSON.
There was a problem hiding this comment.
There's no particular reason; I think the goal here was just to get something in initially so that the flag wasn't just sitting around as dead code. After some discussion, we've decided to go more in the direction that Mark suggested and lean on downstream consumers of the JSON (specifically cargo) for the specific metrics included in this PR. For now, we're going to focus on the ICE reporting mechanism as the initial metric to coexist with the flag since that's actually the one we care about the most.
| let metrics = sopts.unstable_opts.error_metrics.as_ref().and_then(|path| { | ||
| let mut path = path.clone(); | ||
| std::fs::create_dir_all(&path).ok()?; | ||
| path.push(format!("error_metrics_{}", std::process::id())); |
There was a problem hiding this comment.
Why PID? Can we use something more ordered such as YYYY-MM-DD-HH-SS-PID?
There was a problem hiding this comment.
I am not sure of the precise reasoning behind using PID, but I have no objections to switching to datetime-pid. We can definitely use that instead.
|
Does this need to be in rustc? What is the intended consumption workflow (is there intent to stabilize this)? I would expect that ~all of this information is already available to programs running rustc with errors emitted as JSON (e.g., Cargo) and they could handle aggregating and propagating that information. |
|
☔ The latest upstream changes (presumably #119621) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@estebank any updates on this? thanks |
|
Hey y'all. I'm going to be picking up some of this work and wanted to give a quick update on the next steps I'm planning and respond to the questions in the thread.
@Mark-Simulacrum, the specific metrics included in this PR, such as the counts of specific error codes and subdiagnostics, definitely don't need to live in rustc, and we have no objection to moving those downstream to the consumers of the JSON output. There will still need to be some machinery in rustc for metrics we care about that don't make sense to emit to cargo or other consumers, such as saving ICE reports or logging for events that we might turn into assertions in the future (e.g. incremental compilation and situations like https://blog.rust-lang.org/2021/05/10/Rust-1.52.1.html.
The plans for consumption workflows are mostly forming at the moment; we've thrown around ideas like having cargo plugins to summarize information, asking users to grep metrics for specific events on nightly, building support within crater/perf to search for issues in the metrics output, or having rust-analyzer look for issues and notify users when they've encountered a problem we'd like reported. I imagine the direction we go will depend heavily on the issues that we first start leveraging the metrics to track.
The only part we anticipate wanting to stabilize is the flag itself, as per the MCP, which configures the directory where metrics should be stored. Plan going forwardI will close this PR and open a new one that adds the flag and uses it as the default location for saving ICE reports, assuming the |
|
Closing this as per #119355 (comment) |
Add nightly only flag to store information about encountered errors to disk. The information being tracked is: