Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate events happening in multiple threads simultaneously #2845

Merged
merged 7 commits into from Jul 19, 2023

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jul 17, 2023

📜 Description

Introduce new DeduplicateMultithreadedEventProcessor which keeps an in-memory cache of crashed events in a form of map/dict (crash type - thread id). If a crash type has already been processed once and coming in again from a different thread, the processor will drop the event.

This can be the case for OutOfMemoryError, when multiple threads are trying to allocate new memory and therefore crashing within the same millisecond from multiple threads. When this happens, the events are almost equal, except the thread id and timestamp. We've seen this for one of our customers and also were somewhat able to reproduce it locally.

💡 Motivation and Context

Closes #2827

💚 How did you test it?

Manually and automated

📝 Checklist

  • I reviewed the submitted code.
  • 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.
  • 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

final EventDropReason eventDropReason = HintUtils.getEventDropReason(hint);
// in case the event has been dropped by multithreaded deduplicator, the other threads will
// crash the app without a chance to persist the main event so we have to special-case this
if (!isEventDropped
Copy link
Member Author

Choose a reason for hiding this comment

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

The change is needed because otherwise the other crashing threads will not hold a lock anymore and will crash the app immediately, not giving a chance to persist the crash that was the first one (the one which we're going to deduplicate).

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 391.37 ms 407.69 ms 16.32 ms
Size 1.72 MiB 2.29 MiB 575.23 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
496bdfd 272.86 ms 407.33 ms 134.48 ms
f274c79 313.96 ms 355.96 ms 42.00 ms
d0c08e7 310.28 ms 350.68 ms 40.40 ms
caf50ec 302.36 ms 325.25 ms 22.89 ms
8820c5c 330.60 ms 416.86 ms 86.26 ms
adf8fe3 300.49 ms 357.36 ms 56.87 ms
9246ed4 281.79 ms 352.08 ms 70.29 ms
f274c79 334.86 ms 348.54 ms 13.68 ms
dc67004 273.86 ms 346.37 ms 72.51 ms
3baa73f 267.45 ms 388.30 ms 120.85 ms

App size

Revision Plain With Sentry Diff
496bdfd 1.72 MiB 2.28 MiB 571.82 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
d0c08e7 1.72 MiB 2.29 MiB 574.68 KiB
caf50ec 1.72 MiB 2.29 MiB 575.24 KiB
8820c5c 1.72 MiB 2.28 MiB 571.82 KiB
adf8fe3 1.72 MiB 2.29 MiB 575.24 KiB
9246ed4 1.72 MiB 2.28 MiB 572.22 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
dc67004 1.72 MiB 2.28 MiB 573.45 KiB
3baa73f 1.72 MiB 2.29 MiB 575.52 KiB

Previous results on branch: rz/fix/dedupe-ooms

Startup times

Revision Plain With Sentry Diff
c97d70e 305.20 ms 386.12 ms 80.92 ms
fb09880 315.44 ms 372.08 ms 56.64 ms
6772551 274.81 ms 327.41 ms 52.60 ms

App size

Revision Plain With Sentry Diff
c97d70e 1.72 MiB 2.29 MiB 575.82 KiB
fb09880 1.72 MiB 2.29 MiB 575.23 KiB
6772551 1.72 MiB 2.29 MiB 575.82 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.

Nice one, LGTM!

@romtsn romtsn enabled auto-merge (squash) July 19, 2023 09:45
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 94.59% and project coverage change: +0.05 🎉

Comparison is base (288f538) 81.31% compared to head (9f0d9d5) 81.37%.

❗ Current head 9f0d9d5 differs from pull request most recent head 9bc6ed1. Consider uploading reports for the commit 9bc6ed1 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2845      +/-   ##
============================================
+ Coverage     81.31%   81.37%   +0.05%     
- Complexity     4619     4633      +14     
============================================
  Files           352      354       +2     
  Lines         17030    17064      +34     
  Branches       2302     2307       +5     
============================================
+ Hits          13848    13885      +37     
+ Misses         2232     2229       -3     
  Partials        950      950              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/TypeCheckHint.java 0.00% <ø> (ø)
...sentry/DeduplicateMultithreadedEventProcessor.java 92.30% <92.30%> (ø)
sentry/src/main/java/io/sentry/SentryEvent.java 89.43% <100.00%> (+0.07%) ⬆️
...io/sentry/UncaughtExceptionHandlerIntegration.java 90.41% <100.00%> (+7.31%) ⬆️
...src/main/java/io/sentry/hints/EventDropReason.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/util/HintUtils.java 80.00% <100.00%> (+1.87%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@romtsn romtsn merged commit f60279b into main Jul 19, 2023
19 checks passed
@romtsn romtsn deleted the rz/fix/dedupe-ooms branch July 19, 2023 09:58
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.

Duplicate crashes per session in OOM situations
2 participants