From 343ada13a3c565d2642996f1a01e58759f479893 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 3 Oct 2022 15:54:41 +0200 Subject: [PATCH 1/2] Snapshot frames onActivityStarted and on transaction finish then calculate delta --- .../android/core/ActivityFramesTracker.java | 105 ++++++++++++++---- .../android/core/ActivityFramesTrackerTest.kt | 83 ++++++++++++-- 2 files changed, 158 insertions(+), 30 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java index d71cab5213c..d46d47b7f9b 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java @@ -10,6 +10,7 @@ import io.sentry.protocol.SentryId; import java.util.HashMap; import java.util.Map; +import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -27,6 +28,8 @@ public final class ActivityFramesTracker { private final @NotNull Map> activityMeasurements = new ConcurrentHashMap<>(); + private final @NotNull Map frameCountAtStartSnapshots = + new WeakHashMap<>(); public ActivityFramesTracker(final @NotNull LoadClass loadClass, final @Nullable ILogger logger) { androidXAvailable = @@ -55,33 +58,50 @@ public synchronized void addActivity(final @NotNull Activity activity) { return; } frameMetricsAggregator.add(activity); + snapshotFrameCountsAtStart(activity); } - @SuppressWarnings("NullAway") - public synchronized void setMetrics( - final @NotNull Activity activity, final @NotNull SentryId sentryId) { + private void snapshotFrameCountsAtStart(final @NotNull Activity activity) { + FrameCounts frameCounts = calculateCurrentFrameCounts(); + if (frameCounts != null) { + frameCountAtStartSnapshots.put(activity, frameCounts); + } + } + + private @Nullable FrameCounts diffFrameCountsAtEnd(final @NotNull Activity activity) { + @Nullable + final FrameCounts frameCountsAtStartFromMap = frameCountAtStartSnapshots.remove(activity); + @NotNull + final FrameCounts frameCountsAtStart = + frameCountsAtStartFromMap == null ? new FrameCounts(0, 0, 0) : frameCountsAtStartFromMap; + + @Nullable final FrameCounts frameCountsAtEnd = calculateCurrentFrameCounts(); + if (frameCountsAtEnd == null) { + return null; + } + + final int diffTotalFrames = frameCountsAtEnd.totalFrames - frameCountsAtStart.totalFrames; + final int diffSlowFrames = frameCountsAtEnd.slowFrames - frameCountsAtStart.slowFrames; + final int diffFrozenFrames = frameCountsAtEnd.frozenFrames - frameCountsAtStart.frozenFrames; + + return new FrameCounts(diffTotalFrames, diffSlowFrames, diffFrozenFrames); + } + + private @Nullable FrameCounts calculateCurrentFrameCounts() { if (!isFrameMetricsAggregatorAvailable()) { - return; + return null; + } + + if (frameMetricsAggregator == null) { + return null; } + final @Nullable SparseIntArray[] framesRates = frameMetricsAggregator.getMetrics(); int totalFrames = 0; int slowFrames = 0; int frozenFrames = 0; - SparseIntArray[] framesRates = null; - try { - framesRates = frameMetricsAggregator.remove(activity); - } catch (Throwable ignored) { - // throws IllegalArgumentException when attempting to remove OnFrameMetricsAvailableListener - // that was never added. - // there's no contains method. - // throws NullPointerException when attempting to remove OnFrameMetricsAvailableListener and - // there was no - // Observers, See - // https://android.googlesource.com/platform/frameworks/base/+/140ff5ea8e2d99edc3fbe63a43239e459334c76b - } - - if (framesRates != null) { + if (framesRates != null && framesRates.length > 0) { final SparseIntArray totalIndexArray = framesRates[FrameMetricsAggregator.TOTAL_INDEX]; if (totalIndexArray != null) { for (int i = 0; i < totalIndexArray.size(); i++) { @@ -100,13 +120,53 @@ public synchronized void setMetrics( } } - if (totalFrames == 0 && slowFrames == 0 && frozenFrames == 0) { + return new FrameCounts(totalFrames, slowFrames, frozenFrames); + } + + private static final class FrameCounts { + private final int totalFrames; + private final int slowFrames; + private final int frozenFrames; + + private FrameCounts(int totalFrames, int slowFrames, int frozenFrames) { + this.totalFrames = totalFrames; + this.slowFrames = slowFrames; + this.frozenFrames = frozenFrames; + } + } + + @SuppressWarnings("NullAway") + public synchronized void setMetrics( + final @NotNull Activity activity, final @NotNull SentryId sentryId) { + if (!isFrameMetricsAggregatorAvailable()) { + return; + } + + try { + // NOTE: removing an activity does not reset the frame counts, only reset() does + frameMetricsAggregator.remove(activity); + } catch (Throwable ignored) { + // throws IllegalArgumentException when attempting to remove OnFrameMetricsAvailableListener + // that was never added. + // there's no contains method. + // throws NullPointerException when attempting to remove OnFrameMetricsAvailableListener and + // there was no + // Observers, See + // https://android.googlesource.com/platform/frameworks/base/+/140ff5ea8e2d99edc3fbe63a43239e459334c76b + } + + final @Nullable FrameCounts frameCounts = diffFrameCountsAtEnd(activity); + + if (frameCounts == null + || (frameCounts.totalFrames == 0 + && frameCounts.slowFrames == 0 + && frameCounts.frozenFrames == 0)) { return; } - final MeasurementValue tfValues = new MeasurementValue(totalFrames, NONE_UNIT); - final MeasurementValue sfValues = new MeasurementValue(slowFrames, NONE_UNIT); - final MeasurementValue ffValues = new MeasurementValue(frozenFrames, NONE_UNIT); + final MeasurementValue tfValues = new MeasurementValue(frameCounts.totalFrames, NONE_UNIT); + final MeasurementValue sfValues = new MeasurementValue(frameCounts.slowFrames, NONE_UNIT); + final MeasurementValue ffValues = new MeasurementValue(frameCounts.frozenFrames, NONE_UNIT); final Map measurements = new HashMap<>(); measurements.put("frames_total", tfValues); measurements.put("frames_slow", sfValues); @@ -132,6 +192,7 @@ public synchronized void setMetrics( public synchronized void stop() { if (isFrameMetricsAggregatorAvailable()) { frameMetricsAggregator.stop(); + frameMetricsAggregator.reset(); } activityMeasurements.clear(); } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt index 0e3b0e437b9..b884c365c9a 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt @@ -6,6 +6,8 @@ import androidx.core.app.FrameMetricsAggregator import androidx.test.ext.junit.runners.AndroidJUnit4 import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.times +import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.whenever import io.sentry.ILogger import io.sentry.protocol.SentryId @@ -34,8 +36,9 @@ class ActivityFramesTrackerTest { val sut = fixture.getSut() val array = getArray() - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(emptyArray(), array) + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -48,10 +51,12 @@ class ActivityFramesTrackerTest { @Test fun `sets frozen frames`() { val sut = fixture.getSut() - val array = getArray(frameTime = 705, numFrames = 5) + val arrayAtStart = getArray(frameTime = 705, numFrames = 5) + val arrayAtEnd = getArray(frameTime = 705, numFrames = 10) - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(arrayAtStart, arrayAtEnd) + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -64,10 +69,12 @@ class ActivityFramesTrackerTest { @Test fun `sets slow frames`() { val sut = fixture.getSut() - val array = getArray(frameTime = 20, numFrames = 5) + val arrayAtStart = getArray(frameTime = 20, numFrames = 5) + val arrayAtEnd = getArray(frameTime = 20, numFrames = 10) - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(arrayAtStart, arrayAtEnd) + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -86,8 +93,35 @@ class ActivityFramesTrackerTest { arrayAll.put(705, 6) val array = arrayOf(arrayAll) - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(emptyArray(), array) + + sut.addActivity(fixture.activity) + sut.setMetrics(fixture.activity, fixture.sentryId) + + val metrics = sut.takeMetrics(fixture.sentryId) + + val totalFrames = metrics!!["frames_total"] + assertEquals(totalFrames!!.value, 111f) + + val frozenFrames = metrics["frames_frozen"] + assertEquals(frozenFrames!!.value, 6f) + + val slowFrames = metrics["frames_slow"] + assertEquals(slowFrames!!.value, 5f) + } + + @Test + fun `sets slow and frozen frames even if start was null`() { + val sut = fixture.getSut() + val arrayAll = SparseIntArray() + arrayAll.put(1, 100) + arrayAll.put(20, 5) + arrayAll.put(705, 6) + val array = arrayOf(arrayAll) + + whenever(fixture.aggregator.metrics).thenReturn(null, array) + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -109,8 +143,23 @@ class ActivityFramesTrackerTest { arrayAll.put(0, 0) val array = arrayOf(arrayAll) - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(array) + sut.addActivity(fixture.activity) + sut.setMetrics(fixture.activity, fixture.sentryId) + + val metrics = sut.takeMetrics(fixture.sentryId) + + assertNull(metrics) + } + + @Test + fun `do not set metrics if values are null`() { + val sut = fixture.getSut() + + whenever(fixture.aggregator.metrics).thenReturn(null) + + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -134,9 +183,18 @@ class ActivityFramesTrackerTest { sut.setMetrics(fixture.activity, fixture.sentryId) } + @Test + fun `addActivity and setMetrics combined do not throw if no AndroidX`() { + whenever(fixture.loadClass.isClassAvailable(any(), any())).thenReturn(false) + val sut = ActivityFramesTracker(fixture.loadClass) + + sut.addActivity(fixture.activity) + sut.setMetrics(fixture.activity, fixture.sentryId) + } + @Test fun `setMetrics does not throw if Activity is not added`() { - whenever(fixture.aggregator.remove(any())).thenThrow(IllegalArgumentException()) + whenever(fixture.aggregator.metrics).thenThrow(IllegalArgumentException()) val sut = ActivityFramesTracker(fixture.loadClass) sut.setMetrics(fixture.activity, fixture.sentryId) @@ -150,6 +208,15 @@ class ActivityFramesTrackerTest { sut.stop() } + @Test + fun `stop resets frameMetricsAggregator`() { + val sut = fixture.getSut() + + sut.stop() + + verify(fixture.aggregator, times(1)).reset() + } + @Test fun `takeMetrics returns null if no AndroidX`() { whenever(fixture.loadClass.isClassAvailable(any(), any())).thenReturn(false) From 6044b4ce470e190acea78f4e79c1dd797f3fc6a7 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 3 Oct 2022 16:01:07 +0200 Subject: [PATCH 2/2] Add changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5608d7d32f..c7beba0a8dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix slow and frozen frames tracking ([#2271](https://github.com/getsentry/sentry-java/pull/2271)) + ## 6.4.2 ### Fixes