feat!: API change to allow (in the future) stream writing of trace files #26
Conversation
It does not support streaming, yet.
…TraceWriter trait
…from runtime_tracing
…e_trace_events(). Instead, use the format, specified in the previous call to begin_writing_trace_events()
…trace_events(). Use the path, specified in the previous call to begin_writing_trace_events() instead
…ents is called without begin_writing_trace_events
…riting_trace_paths
…e_metadata; use the path, specified in the call to begin_writing_trace_metadata
…e_paths; use the path, specified in the call to begin_writing_trace_paths
| impl TraceWriter for Tracer { | ||
| /// Begin tracing of a program starting at the given source location. | ||
| pub fn start(&mut self, path: &Path, line: Line) { | ||
| fn start(&mut self, path: &Path, line: Line) { |
There was a problem hiding this comment.
i didn't realize i don't need to add pub for pub trait method implementations before: nice
There was a problem hiding this comment.
Yes. Actually, when implementing a trait, using 'pub' is not allowed and produces a compiler error.
| TraceEventsFileFormat::Binary => { | ||
| let mut file = fs::File::create(path)?; | ||
| crate::capnptrace::write_trace(&self.events, &mut file)?; | ||
| fn finish_writing_trace_events(&mut self) -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
what would this method do for streaming writer in the future: just doing nothing, as everything is already streamed into the file?
There was a problem hiding this comment.
Depends on the file format. For simple file formats, it might be nothing, but for other formats it could also seek back to the header, update the events count, append an index at the end of the file, etc.
There was a problem hiding this comment.
i am asking, because e.g. if we want to start replay-s of partial records, while the recording is still running in the future, it might be good for the record to be always in "usable" form, but this is not something that's a priority now I think. (I remember @zah talking about this in the past)
There was a problem hiding this comment.
This is an entirely independent feature. Adding finish_writing_trace_events() does not prevent us from implementing partial recording support. Even in that case, finish_writing_trace_events() can be useful, for marking the file as finished. And even it's implemented as no operation for some formats, so what? It only takes an extra nanosecond once per finished recording. The extra flexibility is worth it, even for the internal debug log capabilities, e.g. for answering the question "did the tracer crash, or finish successfully".
There was a problem hiding this comment.
yes, i agree it's still useful! just wanted to make sure we can easily support partial records in the future: sounds good
There was a problem hiding this comment.
Partial recording will probably not work at the moment, since we're planning to use Zstd_seekable compression, and I don't think its compression format supports it. I'm not 100% sure about that, though. But that's an independent issue, not related to the API.
|
can you just change the version of the lib to |
|
on the other hand if we want to follow semver .. many of those should be actually updating the |
Done. |
This change still doesn't implement stream writing, yet. It just changes the API, so that future format implementations can support streaming transparently, without any changes to the users of the library.