Optimize DuplicateEventDetectionEventProcessor performance. #1247

maciejwalkowiak · 2021-02-12T17:44:54Z

📜 Description

Optimize potential DuplicateEventDetectionEventProcessor performance issue by changing how objects are stored from WeakHashMap to ConcurrentLinkedQueue.

💡 Motivation and Context

Using WeakHashMap to store objects opens up a possibility that the map will grow to thousands of entries and causes performance loss when searching for stored exception. This may happen either if outside of the SDK code references to exceptions are kept or if GC does not run for long time.

Changing the implementation to ConcurrentLinkedQueue gives the event processor control over how many exceptions are stored in the memory.

💚 How did you test it?

Unit tests.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

Using WeakHashMap to store objects opens up a possibility that the map will grow to thousands of entries and causes performance loss when searching for stored exception. This may happen either if outside of the SDK code references to exceptions are kept or if GC does not run for long time. Changing the implementation to `ConcurrentLinkedDeque` gives the event processor control over how many exceptions are stored in the memory.

bruno-garcia · 2021-02-12T21:55:34Z

sentry/src/main/java/io/sentry/DuplicateEventDetectionEventProcessor.java

  }

  @Override
  public SentryEvent process(final @NotNull SentryEvent event, final @Nullable Object hint) {
    final Throwable throwable = event.getOriginThrowable();
    if (throwable != null) {
-      if (capturedObjects.containsKey(throwable)
+      if (capturedObjects.contains(throwable)


Now I read this allCauses, doesn't seem optimal.

Even with this refactor, if exceptions have many causes we'll be allocating new lists and running this thing for each captureException, and doing the lookup for each one of those. The 100 limit is multiplied by the number of inner exceptions.

Do we even need to allocate a list there? That while could be inlined here and the contains could be checked on each iteration.

And with this refactor, we'll build up a list of 100 and always keep them around, even long after the exception was collected. This means for a long running process we'll always lookup on the full map. Still an improvement in case a service is throwing a ton of exceptions and for some reason it doesn't run GC because it has TB of RAM but does sound suboptimal to other scenarios.

Not sure if I understand. allCauses is executed on the exception that is being captured now. We are not looking into causes of all previously captured exceptions.

And with this refactor, we'll build up a list of 100 and always keep them around, even long after the exception was collected. This means for a long running process we'll always lookup on the full map. Still an improvement in case a service is throwing a ton of exceptions and for some reason it doesn't run GC because it has TB of RAM but does sound suboptimal to other scenarios.

That's true. Do you have a suggestion that would work in all scenarios? We can also store exceptions along with the time they were captured, and once DuplicateEventProcessor while iterating for checking for equality, check also if it's not older than X, if so - remove.

marandaneto · 2021-02-16T08:26:22Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Enchancement: Support @SentrySpan and @SentryTransaction on classes and interfaces. (#1243)
 * Enchancement: Do not serialize empty collections and maps (#1245)
 * Ref: Simplify RestTemplate instrumentation (#1246)
+* Ref: Optimize DuplicateEventDetectionEventProcessor performance (#1247).


should also go under the breaking change section

It's changing the algorithm that sits under the hood. I am not sure why this would be qualified as a breaking change.

its a behavior change, there's a bufferSize now and there is the possibility to report the same exception twice, right?

Unlikely, only if applications sends hundreds/thousands errors per second. Buffer size is hidden from the user. I can add it to breaking change if you like but I don't believe we should bump a version because of this.

…n-event-processor

sentry/src/main/java/io/sentry/DuplicateEventDetectionEventProcessor.java

codecov-io · 2021-02-16T17:55:04Z

Codecov Report

Merging #1247 (4d21932) into main (9003653) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1247      +/-   ##
============================================
+ Coverage     74.91%   74.95%   +0.04%     
- Complexity     1743     1748       +5     
============================================
  Files           183      183              
  Lines          6118     6130      +12     
  Branches        609      610       +1     
============================================
+ Hits           4583     4595      +12     
  Misses         1256     1256              
  Partials        279      279

Impacted Files	Coverage Δ	Complexity Δ
.../sentry/DuplicateEventDetectionEventProcessor.java	`100.00% <100.00%> (ø)`	`11.00 <6.00> (+1.00)`
sentry/src/main/java/io/sentry/SentryOptions.java	`85.06% <100.00%> (+0.42%)`	`119.00 <3.00> (+4.00)`
sentry/src/main/java/io/sentry/Scope.java	`92.82% <0.00%> (ø)`	`65.00% <0.00%> (ø%)`
sentry/src/main/java/io/sentry/Attachment.java	`100.00% <0.00%> (ø)`	`12.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9003653...4d21932. Read the comment docs.

bruno-garcia · 2021-02-16T18:18:15Z

sentry/src/main/java/io/sentry/DuplicateEventDetectionEventProcessor.java


 /** Deduplicates events containing throwable that has been already processed. */
 public final class DuplicateEventDetectionEventProcessor implements EventProcessor {
-  private final WeakHashMap<Throwable, Object> capturedObjects = new WeakHashMap<>();
+  private final ConcurrentLinkedQueue<Throwable> capturedObjects = new ConcurrentLinkedQueue<>();


I'm not confortable with this to be honest. Can we discuss this before going ahead?

Sure sure, I was waiting for your blessing anyway :-) We must change the WeakHashMap though (https://kdowbecki.github.io/WeakHashMap-is-not-thread-safe/ - this likely contributed to our issues). The option could be to synchronize WeakHashMap but this comes with drawbacks too.

There's no thread safe WeakHashMap in Java standard library, but perhaps we can include this dependency? https://github.com/raphw/weak-lock-free

Or: considering that Deduplication is neccessary only with Spring + logging framework integration, perhaps we can move this event processor to Spring module and use ConcurrentReferenceHashMap from Spring Framework. This would solve the problem assuming that only thread safety is an issue.

We discussed this on a call and decided to go with @maciejwalkowiak's suggestion of using Collections.synchronizedMap.

maciejwalkowiak · 2021-02-17T10:01:58Z

WeakHashMap is not thread safe and can end up in endless loop. We've discussed with @bruno-garcia to synchronize map. This should make the performance issue go away.

maciejwalkowiak · 2021-02-17T10:56:15Z

@marandaneto take a look please at the recent changes. What do you think about this implementation?

marandaneto · 2021-02-18T08:56:17Z

sentry/src/test/java/io/sentry/DuplicateEventDetectionEventProcessorTest.kt

+            val options = SentryOptions().apply {
+                if (enableDeduplication != null) {
+                    this.setEnableDeduplication(enableDeduplication)
+                }
+            }


you could init SentryOptions outside of getSut but still inside of Fixture and call options setters directly on each test, I think that's the way we've been doing so far.
getSut only takes params if the Sut (eg DuplicateEventDetectionEventProcessor) itself requires those params

marandaneto · 2021-02-18T08:58:23Z

@marandaneto take a look please at the recent changes. What do you think about this implementation?

looks good to me, left a comment about the testing, its a nit though, feel free to ignore.
update the PR template please, its outdated now.
Also, let's add enable-deduplication to the docs

maciejwalkowiak added 3 commits February 12, 2021 18:07

Fix Android compatiblility.

7c2b411

Changelog.

d6b75c9

maciejwalkowiak requested review from bruno-garcia and marandaneto as code owners February 12, 2021 17:44

Changelog.

fe1c9c1

bruno-garcia reviewed Feb 12, 2021

View reviewed changes

marandaneto reviewed Feb 16, 2021

View reviewed changes

maciejwalkowiak added 3 commits February 16, 2021 17:52

Merge remote-tracking branch 'origin/main' into optimize-deduplicatio…

9d46cfb

…n-event-processor

Add a flag to disable deduplication.

a66b5ad

Api.

0497e68

marandaneto reviewed Feb 16, 2021

View reviewed changes

sentry/src/main/java/io/sentry/DuplicateEventDetectionEventProcessor.java Show resolved Hide resolved

marandaneto approved these changes Feb 16, 2021

View reviewed changes

bruno-garcia requested changes Feb 16, 2021

View reviewed changes

maciejwalkowiak added 2 commits February 17, 2021 10:58

Synchronize WeakHashMap.

065e73e

Log when deduplication is disabled.

4d21932

Merge branch 'main' into optimize-deduplication-event-processor

68e4929

marandaneto reviewed Feb 18, 2021

View reviewed changes

marandaneto requested a review from bruno-garcia February 18, 2021 08:56

maciejwalkowiak merged commit cf436a0 into main Feb 18, 2021

maciejwalkowiak deleted the optimize-deduplication-event-processor branch February 18, 2021 14:06

dependabot bot mentioned this pull request Mar 14, 2021

Bump sentry from 3.1.1 to 4.3.0 LinwoodDev/BotJava#71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize DuplicateEventDetectionEventProcessor performance. #1247

Optimize DuplicateEventDetectionEventProcessor performance. #1247

maciejwalkowiak commented Feb 12, 2021

bruno-garcia Feb 12, 2021

maciejwalkowiak Feb 15, 2021

marandaneto Feb 16, 2021

maciejwalkowiak Feb 16, 2021

marandaneto Feb 16, 2021

maciejwalkowiak Feb 16, 2021

codecov-io commented Feb 16, 2021 •

edited

bruno-garcia Feb 16, 2021

maciejwalkowiak Feb 16, 2021

maciejwalkowiak Feb 16, 2021

maciejwalkowiak Feb 16, 2021

bruno-garcia Feb 16, 2021

maciejwalkowiak commented Feb 17, 2021

maciejwalkowiak commented Feb 17, 2021

marandaneto Feb 18, 2021

marandaneto commented Feb 18, 2021 •

edited

Optimize DuplicateEventDetectionEventProcessor performance. #1247

Optimize DuplicateEventDetectionEventProcessor performance. #1247

Conversation

maciejwalkowiak commented Feb 12, 2021

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 16, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejwalkowiak commented Feb 17, 2021

maciejwalkowiak commented Feb 17, 2021

Choose a reason for hiding this comment

marandaneto commented Feb 18, 2021 • edited

codecov-io commented Feb 16, 2021 •

edited

marandaneto commented Feb 18, 2021 •

edited