Skip to content

Conversation

@supervacuus
Copy link
Collaborator

@supervacuus supervacuus commented Jan 19, 2026

📜 Description

This implements the second aspect of the tombstone integration: merging tombstone events with crash events coming from sentry-native. It does this in the following way:

  • No changes to the OutboxSender, but instead let TombstoneIntegration own the outbox folder before the OutboxSender operates on it (i.e., we rely on integration instantiation order and the fact that all async integration work is done on a single-threaded executor service).
  • NativeEventCollector implements the duplication wrt OutboxSender, but specifically for handling envelopes that contain native crashes
  • The upside is that we don't have to touch non-Android-specific code without making it more generic. If it turns out we want to extend OutboxSender later, that change wouldn't be a considerable effort.
  • Introduces a correlation id that allows us to map exactly between native SDK events and tombstones (vs correlating via timestamp). This requires changes to sentry-native, so for now it's only exposed but not used.
  • TombstonePolicy searches, matches, and merges events
  • merging is essentially: take the native SDK event and replace debugMeta, exception, and threads
  • Right now, these events will go through the same enrichment as the tombstones, which might or might not be the right choice

This is primarily an end-to-end implementation to discuss the trade-offs listed.

💡 Motivation and Context

This is the last essential step before releasing the tombstone integration to a wider audience. It maps directly to
Implement merge mechanism with the Native SDK envelope

💚 How did you test it?

  • only manually up to now
  • Once the direction is agreed on, will add unit+integration tests

📝 Checklist

  • I added GH Issue ID & Linear ID
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • decide on this approach or extending OutboxSender and surrounding infra more generically (we could introduce a callback for the core of the OutboxSender decision and set up integrations that use it accordingly)
  • decide merging strategy
  • decide on enrichment (maybe we should use a more default EventProcessor for merge events?)
  • Related to the above:
    • Should correlation only be attempted for the latest tombstone?
    • How should historical tombstones be matched? (Since we have all the actual data from the moment of the event in the native envelope, they will likely be more accurate for that moment in time vs backfilling, but we don't track the same amount of context information)
  • Add correlation id to the Native SDK

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2026

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


New Features ✨

  • Merge tombstone and native sdk events by supervacuus in #5037

Bug Fixes 🐛

  • Establish native exception mechanisms by supervacuus in #5052

Internal Changes 🔧

  • (android) Update targetSdk to API 36 (Android 16) by markushi in #5016
  • (ci) Write permission for statuses in changelog preview by supervacuus in #5053

🤖 This preview updates automatically when you update the PR.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2026

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against ba1bfc7

@supervacuus
Copy link
Collaborator Author

@markushi and @romtsn, can you have a first look so we can discuss trade-offs? Thx!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2026

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 322.85 ms 367.92 ms 45.07 ms
Size 1.58 MiB 2.19 MiB 622.94 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dba088c 333.98 ms 381.16 ms 47.18 ms
91bb874 310.68 ms 359.24 ms 48.56 ms
d15471f 315.61 ms 360.22 ms 44.61 ms
27d7cf8 314.17 ms 347.00 ms 32.83 ms
a5ab36f 316.83 ms 394.54 ms 77.71 ms
5b66efd 308.67 ms 363.85 ms 55.18 ms
abfcc92 337.38 ms 427.39 ms 90.00 ms
fc5ccaf 279.11 ms 353.34 ms 74.23 ms
fcec2f2 328.91 ms 387.75 ms 58.84 ms
dba088c 365.46 ms 366.31 ms 0.85 ms

App size

Revision Plain With Sentry Diff
dba088c 1.58 MiB 2.13 MiB 558.99 KiB
91bb874 1.58 MiB 2.13 MiB 559.07 KiB
d15471f 1.58 MiB 2.13 MiB 559.54 KiB
27d7cf8 1.58 MiB 2.12 MiB 549.42 KiB
a5ab36f 1.58 MiB 2.12 MiB 555.26 KiB
5b66efd 1.58 MiB 2.13 MiB 559.07 KiB
abfcc92 1.58 MiB 2.13 MiB 557.31 KiB
fc5ccaf 1.58 MiB 2.13 MiB 557.54 KiB
fcec2f2 1.58 MiB 2.12 MiB 551.50 KiB
dba088c 1.58 MiB 2.13 MiB 558.99 KiB

Previous results on branch: feat/tombstone_native_sdk_merge

Startup times

Revision Plain With Sentry Diff
7aef08b 286.72 ms 361.82 ms 75.09 ms
14ebc28 320.09 ms 398.66 ms 78.57 ms
6ad3f8e 313.98 ms 362.27 ms 48.29 ms

App size

Revision Plain With Sentry Diff
7aef08b 1.58 MiB 2.19 MiB 622.25 KiB
14ebc28 1.58 MiB 2.20 MiB 637.07 KiB
6ad3f8e 1.58 MiB 2.19 MiB 622.25 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking quite promising already, I think this is the right track. Left a few comments for further discussion.

*/
private static void setNativeCrashCorrelationId(
final @NotNull Context context, final @NotNull SentryAndroidOptions options) {
final String correlationId = UUID.randomUUID().toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As anyone could call this API to store information, it might be a good idea to ensure it's our data we're reading back later. I guess a simple prefix could work e.g. sentry-app-run-<uuid>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, and this is only meant as a proposal that survived from 2022:

  • processStateSummary is global and generally owned by the app
  • If we write into this, we should first check if there is already something in there
  • Even if nothing was in there, any later code can overwrite it without us knowing anything about it
  • The maximum length is 128 bytes, and writing into it is free-form, so there is no way for binary stability except if we communicate that Sentry owns this if you use the SDK (not sure if that is a good idea).
  • If we prefix it with something generic like sentry-app-run-, it shows that this might be used beyond native crash correlation (it will be accessible from any ApplicationExitInfo).

I implemented this because we considered it a stable correlation mechanism back then, but I think timestamp correlation is stable enough and doesn't depend on app-owned storage.

It's likely a good idea to drop this altogether, and if we introduce it, maybe together with other ApplicationExitInfo handlers?

.getLogger()
.log(SentryLevel.DEBUG, "Scanning %d files in outbox for native events.", files.length);

for (final File file : files) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried of ending up with too many envelopes in memory here and potentially running into OOMs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me which code in particular is likely to cause references to spill? I'm not entirely sure. My assumption was that there would typically be very few native events (on average, 1 after each native crash), since they are usually sent on the next restart (and we only collect native events and their envelopes). So I would guess that, while iterating over and deserializing all envelopes, each iteration should leave the discarded ones ready for GC, right?

My bigger worry was the attachments that we pass around with the envelopes (yes, they are currently not attached to the tombstone events). I am fine with not passing around the envelope for the event, but we must ensure it is loaded at some point. Was this your concern? We could store only the envelope path in NativeEventData and only load it once we have a matching tombstone. Or did I completely misunderstand?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants