Improve folding logic, cleanup tests

Author: markushi

sentry-android-core/api/sentry-android-core.api

@@ -483,8 +483,8 @@ public final class io/sentry/android/core/ViewHierarchyEventProcessor : io/sentr
 }
 
 public class io/sentry/android/core/anr/AggregatedStackTrace {
-	public fun <init> ([Ljava/lang/StackTraceElement;IIJI)V
-	public fun add (J)V
+	public fun <init> ([Ljava/lang/StackTraceElement;IIJF)V
+	public fun addOccurrence (J)V
 	public fun getStack ()[Ljava/lang/StackTraceElement;
 }
 

sentry-android-core/src/main/java/io/sentry/android/core/anr/AggregatedStackTrace.java

@@ -2,14 +2,15 @@
 
 import java.util.Arrays;
 import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
 
 @ApiStatus.Internal
 public class AggregatedStackTrace {
   // the number of frames of the stacktrace
   final int depth;
 
-  // the quality of the stack trace, higher means better
-  final int quality;
+  // the quality of the stack trace, higher means better (ratio of app frames: 0.0 to 1.0)
+  final float quality;
 
   private final StackTraceElement[] stack;
 
@@ -20,18 +21,18 @@ public class AggregatedStackTrace {
   // the total number of times this exact stacktrace was captured
   int count;
 
-  // first time the stacktrace occured
+  // first time the stacktrace occurred
   private long startTimeMs;
 
-  // last time the stacktrace occured
+  // last time the stacktrace occurred
   private long endTimeMs;
 
   public AggregatedStackTrace(
       final StackTraceElement[] stack,
       final int stackStartIdx,
       final int stackEndIdx,
       final long timestampMs,
-      final int quality) {
+      final float quality) {
     this.stack = stack;
     this.stackStartIdx = stackStartIdx;
     this.stackEndIdx = stackEndIdx;
@@ -42,12 +43,13 @@ public AggregatedStackTrace(
     this.quality = quality;
   }
 
-  public void add(long timestampMs) {
+  public void addOccurrence(final long timestampMs) {
     this.startTimeMs = Math.min(startTimeMs, timestampMs);
     this.endTimeMs = Math.max(endTimeMs, timestampMs);
     this.count++;
   }
 
+  @NotNull
   public StackTraceElement[] getStack() {
     return Arrays.copyOfRange(stack, stackStartIdx, stackEndIdx + 1);
   }

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrCulpritIdentifier.java

@@ -13,22 +13,79 @@
 public class AnrCulpritIdentifier {
 
   // common Java and Android packages who are less relevant for being the actual culprit
-  private static final List<String> lowQualityPackages = new ArrayList<>(9);
+  private static final List<String> systemAndFrameWorkPackages = new ArrayList<>(9);
 
   static {
-    lowQualityPackages.add("java.lang");
-    lowQualityPackages.add("java.util");
-    lowQualityPackages.add("android.app");
-    lowQualityPackages.add("android.os.Handler");
-    lowQualityPackages.add("android.os.Looper");
-    lowQualityPackages.add("android.view");
-    lowQualityPackages.add("android.widget");
-    lowQualityPackages.add("com.android.internal");
-    lowQualityPackages.add("com.google.android");
+    systemAndFrameWorkPackages.add("java.lang");
+    systemAndFrameWorkPackages.add("java.util");
+    systemAndFrameWorkPackages.add("android.app");
+    systemAndFrameWorkPackages.add("android.os.Handler");
+    systemAndFrameWorkPackages.add("android.os.Looper");
+    systemAndFrameWorkPackages.add("android.view");
+    systemAndFrameWorkPackages.add("android.widget");
+    systemAndFrameWorkPackages.add("com.android.internal");
+    systemAndFrameWorkPackages.add("com.google.android");
+  }
+
+  private static final class StackTraceKey {
+    private final @NotNull StackTraceElement[] stack;
+    private final int startIdx;
+    private final int endIdx;
+    private final int hashCode;
+
+    StackTraceKey(final @NotNull StackTraceElement[] stack, final int startIdx, final int endIdx) {
+      this.stack = stack;
+      this.startIdx = startIdx;
+      this.endIdx = endIdx;
+      this.hashCode = computeHashCode();
+    }
+
+    private int computeHashCode() {
+      int result = 1;
+      for (int i = startIdx; i <= endIdx; i++) {
+        result = 31 * result + stack[i].hashCode();
+      }
+      return result;
+    }
+
+    @Override
+    public int hashCode() {
+      return hashCode;
+    }
+
+    @Override
+    public boolean equals(final Object obj) {
+      if (this == obj) {
+        return true;
+      }
+      if (!(obj instanceof StackTraceKey)) {
+        return false;
+      }
+
+      final @NotNull StackTraceKey other = (StackTraceKey) obj;
+
+      if (hashCode != other.hashCode) {
+        return false;
+      }
+
+      final int length = endIdx - startIdx + 1;
+      final int otherLength = other.endIdx - other.startIdx + 1;
+      if (length != otherLength) {
+        return false;
+      }
+
+      for (int i = 0; i < length; i++) {
+        if (!stack[startIdx + i].equals(other.stack[other.startIdx + i])) {
+          return false;
+        }
+      }
+
+      return true;
+    }
   }
 
   /**
-   * @param stacks the captured stacktraces
+   * @param stacks a list of stack traces to analyze
    * @return the most common occurring stacktrace identified as the culprit
    */
   @Nullable
@@ -38,27 +95,32 @@ public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> s
     }
 
     // fold all stacktraces and count their occurrences
-    final @NotNull Map<Integer, AggregatedStackTrace> stackTraceMap = new HashMap<>();
-    for (final AnrStackTrace stackTrace : stacks) {
-
+    final @NotNull Map<StackTraceKey, AggregatedStackTrace> stackTraceMap = new HashMap<>();
+    for (final @NotNull AnrStackTrace stackTrace : stacks) {
       if (stackTrace.stack.length < 2) {
         continue;
       }
 
-      // entry 0 is the most detailed element in the stacktrace
-      // so create sub-stacks (1..n, 2..n, ...) to capture the most common root cause of an ANR
-      for (int i = 0; i < stackTrace.stack.length - 1; i++) {
-        // TODO using hashcode is actually a bad key
-        final int key = subArrayHashCode(stackTrace.stack, i, stackTrace.stack.length - 1);
-        int quality = 10;
-        final String clazz = stackTrace.stack[i].getClassName();
-        for (String ignoredPackage : lowQualityPackages) {
-          if (clazz.startsWith(ignoredPackage)) {
-            quality = 1;
-            break;
-          }
+      // stack[0] is the most detailed element in the stacktrace
+      // iterate from end to start (length-1 → 0) creating sub-stacks (i..n-1) to find the most
+      // common root cause
+      // count app frames from the end to compute quality scores
+      int appFramesCount = 0;
+
+      for (int i = stackTrace.stack.length - 1; i >= 0; i--) {
+
+        final @NotNull String topMostClassName = stackTrace.stack[i].getClassName();
+        final boolean isSystemFrame = isSystemFrame(topMostClassName);
+        if (!isSystemFrame) {
+          appFramesCount++;
         }
 
+        final int totalFrames = stackTrace.stack.length - i;
+        final float quality = (float) appFramesCount / totalFrames;
+
+        final @NotNull StackTraceKey key =
+            new StackTraceKey(stackTrace.stack, i, stackTrace.stack.length - 1);
+
         @Nullable AggregatedStackTrace aggregatedStackTrace = stackTraceMap.get(key);
         if (aggregatedStackTrace == null) {
           aggregatedStackTrace =
@@ -70,7 +132,7 @@ public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> s
                   quality);
           stackTraceMap.put(key, aggregatedStackTrace);
         } else {
-          aggregatedStackTrace.add(stackTrace.timestampMs);
+          aggregatedStackTrace.addOccurrence(stackTrace.timestampMs);
         }
       }
     }
@@ -82,22 +144,16 @@ public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> s
     // the deepest stacktrace with most count wins
     return Collections.max(
         stackTraceMap.values(),
-        (c1, c2) -> {
-          final int countComparison = Integer.compare(c1.count * c1.quality, c2.count * c2.quality);
-          if (countComparison == 0) {
-            return Integer.compare(c1.depth, c2.depth);
-          }
-          return countComparison;
-        });
+        (c1, c2) ->
+            Float.compare(c1.count * c1.quality * c1.depth, c2.count * c2.quality * c2.depth));
   }
 
-  private static int subArrayHashCode(
-      final @NotNull Object[] arr, final int stackStartIdx, final int stackEndIdx) {
-    int result = 1;
-    for (int i = stackStartIdx; i <= stackEndIdx; i++) {
-      final Object item = arr[i];
-      result = 31 * result + item.hashCode();
+  private static boolean isSystemFrame(final @NotNull String clazz) {
+    for (final String systemPackage : systemAndFrameWorkPackages) {
+      if (clazz.startsWith(systemPackage)) {
+        return true;
+      }
     }
-    return result;
+    return false;
   }
 }