feat(memtrack): add zstd compression support#184
Conversation
1afc939 to
f58ee6c
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds zstd compression support to memtrack artifacts to reduce storage requirements. The implementation introduces streaming compression/decompression and refactors the event processing architecture to write events directly to disk during collection rather than accumulating them in memory.
Key changes:
- Introduces
MemtrackWriterwith zstd compression for streaming event serialization - Refactors
track_commandto use a two-stage threading model (drain thread → writer thread) for immediate disk writing - Centralizes filename generation logic in the
ArtifactExttrait
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/runner-shared/src/artifacts/mod.rs | Adds file_name() method to centralize filename generation for artifacts with optional PID |
| crates/runner-shared/src/artifacts/memtrack.rs | Introduces MemtrackWriter struct with zstd compression and updates decoder to decompress streams |
| crates/memtrack/src/main.rs | Refactors event processing to use two-stage threading (drain + writer) for streaming events to compressed disk files |
| crates/runner-shared/Cargo.toml | Adds zstd dependency version 0.13 |
| Cargo.lock | Updates lock file with zstd and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f3e451e to
2cb7cc4
Compare
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| format!("{pid}.{}.msgpack", Self::name()) | ||
| } else { | ||
| format!("{}.msgpack", Self::name()) |
There was a problem hiding this comment.
The file extension .msgpack is misleading now that the files are compressed with zstd. Consider changing the extension to .msgpack.zst or .msgpack.zstd to accurately reflect that the files are compressed, which will help users understand the file format and ensure proper handling.
| format!("{pid}.{}.msgpack", Self::name()) | |
| } else { | |
| format!("{}.msgpack", Self::name()) | |
| format!("{pid}.{}.msgpack.zst", Self::name()) | |
| } else { | |
| format!("{}.msgpack.zst", Self::name()) |
There was a problem hiding this comment.
I agree that it's misleading, but I think it's not worth it changing the file extension. The data format is hidden in the MemtrackWriter and the MemtrackEventStream, so the consumer doesn't care about it.
2cb7cc4 to
35e5f3f
Compare
Changes in this PR: