feat: unconditionally send off a final report on LocalReporter drop#125
feat: unconditionally send off a final report on LocalReporter drop#125jlizen wants to merge 3 commits intoasync-profiler:mainfrom
Conversation
Adds a synchronous report_blocking method with a default no-op that logs a warning. LocalReporter overrides it to copy JFR files via std::fs::copy, and MultiReporter delegates to its children. The method accepts a &Path instead of Vec<u8> so that reporters decide whether and how to read the file, avoiding an unconditional read for reporters that don't support synchronous reporting.
|
I was discussing with @rcoh offline about this seeming fairly complex for what we're trying to do. Here's why it ended up this way. The JFR data lives in anonymous tempfiles owned by I looked at doing this entirely inside The decent options I saw all involved trait methods, just with different control flows.
Do you prefer any of those? Or is there an option I'm missing? |
📬 Issue #, if available:
Closes #113
✍️ Description of changes:
We want the local report to unconditionally flush any JFRs prior to shutting down the runtime. And then, for all other reporters, existing behavior of warning should be preserved.
I added a
report_blockingmethod with a default implementation to non-op. That default is slightly better than the old one, since we no longer warn EVERY drop if no JFRs are omitted (previously, we unconditionally warned).And then, the LocalReport does what you'd expect. As does the multi-reporter, it attempts each internal report's
warncall on drop.For non-local reporter usage, adds about 1 byte of memory overhead per profiler instance between the
completed_normallybool + padding (which lets us improve the warning). And then, there is an extra 8 bytes for the new vtable entry, and an extra vtable pointer chase on the cancellation path.On the non-local-reporter path (e.g. S3 reporter hitting the default no-op),
try_final_reportdoes:/proc/self/fd/NPathBuf from a format string (~20 byte allocation)statsyscall to check file sizeWe could get that further down by leaving off
ReportMetadata, which we don't really use in the local reporterreport_blockinganyway, but it seemed like a good idea to bake in.🔏 By submitting this pull request