fix: Avoid serializing minidumps over procspawn #508
Conversation
| sentry::protocol::Value::String( | ||
| path.to_string_lossy().to_string(), | ||
| ), |
There was a problem hiding this comment.
ugh, we should make the SDK nicer to use ;)
There was a problem hiding this comment.
- Rather than passing in a named temp file, pass around the
File, if possible. The OS should still keep it open. - Stream into a file directly in the endpoint, then pass around the file. @flub we've had various versions of this in the past already, do you remember where this was?
hmm, no i don't. sorry |
|
now streaming directly to a file (and updating rustc), I get these timings, though I noticed they have high variability: before: |
| // constructor on `ProcessResult`. Passing in an empty buffer should result in | ||
| // the same error though. | ||
| let minidump = | ||
| ByteView::open(minidump_path).unwrap_or_else(|_| ByteView::from_slice(b"")); |
There was a problem hiding this comment.
An empty buffer will now result in a generic "failed to read miniump" error with little information for debugging.
Is there no way you can wrap the error somehow?
There was a problem hiding this comment.
I think "minidump not found" is the appropriate error here. Also, not sure what to wrap this with, as also everything needs to go through serde, which std::io::Error does not.
There was a problem hiding this comment.
There is some elegance to writing it to disk straight away... but I'm not sure I really like this more than the solution before which didn't have this filesystem problem because it could directly save in the diagnostics cache. Though thinking of it, this made it impossible to detect what files in the diagnostics cache were actually failed and which ones were life, so maybe that was also not a good solution.
Sorry, only creating problems here it seems, not providing solutions 😕
| if let Some(dir) = self.diagnostics_cache.cache_dir() { | ||
| if let Some(file_name) = minidump_file.file_name() { | ||
| let path = dir.join(file_name); | ||
| match minidump_file.persist(&path) { |
There was a problem hiding this comment.
Note: Temporary files cannot be persisted across filesystems.
Are we sure this is never going to be the case? It often is the case that /tmp is a different filesystem. And afaik our cache directory is a persistent volume so very likely to be a different filesystem to wherever tempfile defaults.
/cc @beezz: can you confirm whether this would work or not?
There was a problem hiding this comment.
I changed this to save the files in $CACHE/tmp which is also used for other tmp files already, so that won’t run into this particular problem.
Instead of passing (and serializing) a byte buffer to procspawn, this will rather create a tempfile and pass that path via procspawn.
This seems to be a small win when processing a 20M minidump, though note that I was timing
cargo run -p process-eventso timings include overhead from both cargo and uploading of the minidump.before:
Time (mean ± σ): 1.611 s ± 0.041 s [User: 274.8 ms, System: 102.0 ms]
Range (min … max): 1.548 s … 1.697 s 10 runs
after:
Time (mean ± σ): 1.526 s ± 0.024 s [User: 275.7 ms, System: 101.5 ms]
Range (min … max): 1.496 s … 1.573 s 10 runs
Due to some error handling and borrowcheck shenanigans, the minidumps are now persisted in the diagnostics cache for all errors during processing, while previously timeouts were ignored.