perf(core): Prototype breadcrumb persistence as a JSONL append log - #5843
Closed
runningcode wants to merge 1 commit into
Closed
perf(core): Prototype breadcrumb persistence as a JSONL append log#5843runningcode wants to merge 1 commit into
runningcode wants to merge 1 commit into
Conversation
…AVA-628) Breadcrumbs are persisted through a vendored copy of Square's tape, a durable transactional FIFO. Neither consumer needs that: both append and read the whole list back, and never dequeue. Tape has also been archived read-only upstream since October 2024, at a 2.0.0-beta1 that never shipped final, and costs ~1200 lines of vendored code plus ~1000 lines of tests. Replace the breadcrumb path with a newline-delimited JSON append log in ~200 lines. Bounding is amortized: once the log passes twice maxBreadcrumbs, it is rewritten down to the newest maxBreadcrumbs behind an atomic rename. Corruption also degrades better. Tape's own header warns that on filesystems without atomic segment writes a mid-write power loss corrupts the file, which is why the old code deleted and recreated it on any IOException. A torn JSONL line fails to deserialize and is skipped, leaving every earlier breadcrumb readable. The benchmark added here does not support the performance case, though. Against tape as configured today the append log is 17-41x faster, but against tape with its per-flush fsync removed it is 1.02x on batched flushes and 0.13x on single-crumb flushes. Nearly all of the win is dropping the fsync, not changing the format — so this stands on maintenance and corruption behaviour, and dropping the fsync from tape would be the cheaper way to get the speed. AnrProfileManager still uses tape, so this deletes no vendored code yet. Prototype for discussion; not intended to merge as-is.
Contributor
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Performance
- Prototype breadcrumb persistence as a JSONL append log ([#5843](https://github.com/getsentry/sentry-java/pull/5843))If none of the above apply, you can opt out of this check by adding |
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1afaf81 | 334.74 ms | 380.29 ms | 45.55 ms |
| 5ad5c95 | 318.13 ms | 364.98 ms | 46.85 ms |
| 8e8cb90 | 313.06 ms | 356.54 ms | 43.48 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1afaf81 | 0 B | 0 B | 0 B |
| 5ad5c95 | 0 B | 0 B | 0 B |
| 8e8cb90 | 0 B | 0 B | 0 B |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Prototype replacing the vendored Square tape
QueueFilewith a newline-delimited JSON append log for breadcrumb persistence. Stacked on #5791.BreadcrumbAppendLog(~200 lines) stores one serialized breadcrumb per line. Appends are a single open/write per flush; bounding is amortized — once the log grows past2 * maxBreadcrumbslines it is rewritten down to the newestmaxBreadcrumbsbehind an atomicrenameTo.AnrProfileManagerstill uses tape, so no vendored code is deleted yet. That would be a follow-up (itsAnrStackTraceusesDataOutputStream, so it needs length-prefixed framing rather than newline framing).💡 Motivation and Context
Tape is a durable transactional FIFO. Neither consumer needs one:
peek()/remove()per-item dequeuetransferTocompactionBoth
PersistingScopeObserverandAnrProfileManageronlyadd,asList, andclear. We're using a transactional queue as an append log, and paying ~1,170 lines of vendored code, ~1,020 lines of tests, aTHIRD_PARTY_NOTICES.mdentry, and a corrupt-file recovery path for it. Upstream has been archived read-only since Oct 2024 at a2.0.0-beta1that never shipped final.Corruption behaviour also improves. Tape's own header warns that on filesystems without atomic segment writes a mid-write power loss "will contain garbage and the file will be corrupt" — which is why the old code caught
IOExceptionand deleted-and-recreated the whole file. A torn trailing JSONL line fails to deserialize and is skipped; every earlier breadcrumb stays readable.The performance case did not hold up
This is the main finding, and it argues against merging as-is. Measured on an M-series Mac, best of 5 after warmup (
BreadcrumbPersistenceBenchmark,@Ignored):Against tape as configured today JSONL is 17–41× faster — but nearly all of that is the per-flush
fsync, not the format. With the fsync removed, tape is 7.9× faster than JSONL on single-crumb flushes and roughly par on batched ones (JSONL reopens the file per flush; tape holds oneRandomAccessFile).So: dropping the
fsyncfrom tape is the cheaper way to get the speed. This PR stands on maintenance burden and corruption behaviour, not throughput.💚 How did you test it?
BreadcrumbAppendLogTest— 17 cases covering round-trip ordering, bounding, amortized compaction, torn trailing lines, corrupt mid-file lines, logs inherited across instances, legacy-format discard, and newline-bearing breadcrumb data (verifying the JSON writer's\nescaping keeps line framing intact).Full
:sentry,:sentry-android-core, and:sentry-android-replayunit test suites pass. Three tests that seeded breadcrumbs in tape's binary format were updated to write JSONL.synchronousWrites(false)+ nosync()on tape and keep the library. That's a much smaller diff.breadcrumbs.jsonfiles are detected by their first byte and deleted, so one launch post-upgrade has no persisted breadcrumbs. Acceptable? (Breadcrumbs are already discarded on init viaresetCache().)BreadcrumbAppendLogis public insentry.apibecausePersistingScopeObserveris in the same package but tests aren't. Could be package-private with a@TestOnlyaccessor.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Decide between this and simply dropping tape's
fsync. If this direction is taken: migrateAnrProfileManagerto length-prefixed framing, then deleteio.sentry.cache.tapeand itsTHIRD_PARTY_NOTICES.mdentry.