diff --git a/CHANGELOG.md b/CHANGELOG.md index a5608d7d32..c7beba0a8d 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 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 d71cab5213..836b561586 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,31 @@ 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 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,31 +101,77 @@ public synchronized void setMetrics( } } - if (totalFrames == 0 && slowFrames == 0 && frozenFrames == 0) { + return new FrameCounts(totalFrames, slowFrames, frozenFrames); + } + + @SuppressWarnings("NullAway") + public synchronized void setMetrics( + final @NotNull Activity activity, final @NotNull SentryId transactionId) { + 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); measurements.put("frames_frozen", ffValues); - activityMeasurements.put(sentryId, measurements); + activityMeasurements.put(transactionId, measurements); + } + + private @Nullable FrameCounts diffFrameCountsAtEnd(final @NotNull Activity activity) { + @Nullable final FrameCounts frameCountsAtStart = frameCountAtStartSnapshots.remove(activity); + if (frameCountsAtStart == null) { + return null; + } + + @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); } @Nullable public synchronized Map takeMetrics( - final @NotNull SentryId sentryId) { + final @NotNull SentryId transactionId) { if (!isFrameMetricsAggregatorAvailable()) { return null; } final Map stringMeasurementValueMap = - activityMeasurements.get(sentryId); - activityMeasurements.remove(sentryId); + activityMeasurements.get(transactionId); + activityMeasurements.remove(transactionId); return stringMeasurementValueMap; } @@ -132,7 +179,20 @@ public synchronized void setMetrics( public synchronized void stop() { if (isFrameMetricsAggregatorAvailable()) { frameMetricsAggregator.stop(); + frameMetricsAggregator.reset(); } activityMeasurements.clear(); } + + private static final class FrameCounts { + private final int totalFrames; + private final int slowFrames; + private final int frozenFrames; + + private FrameCounts(final int totalFrames, final int slowFrames, final int frozenFrames) { + this.totalFrames = totalFrames; + this.slowFrames = slowFrames; + this.frozenFrames = frozenFrames; + } + } } 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 0e3b0e437b..0740fe1121 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 = 701, numFrames = 5) + val arrayAtEnd = getArray(frameTime = 701, 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) @@ -80,14 +87,41 @@ class ActivityFramesTrackerTest { @Test fun `sets slow and frozen frames`() { val sut = fixture.getSut() - val arrayAll = SparseIntArray() - arrayAll.put(1, 100) - arrayAll.put(20, 5) - arrayAll.put(705, 6) - val array = arrayOf(arrayAll) + val array = SparseIntArray().also { + it.put(16, 100) + it.put(20, 5) + it.put(701, 6) + }.let { arrayOf(it) } + + 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 array = SparseIntArray().also { + it.put(16, 100) + it.put(20, 5) + it.put(701, 6) + }.let { arrayOf(it) } - whenever(fixture.aggregator.remove(any())).thenReturn(array) + whenever(fixture.aggregator.metrics).thenReturn(null, array) + sut.addActivity(fixture.activity) sut.setMetrics(fixture.activity, fixture.sentryId) val metrics = sut.takeMetrics(fixture.sentryId) @@ -102,6 +136,122 @@ class ActivityFramesTrackerTest { assertEquals(slowFrames!!.value, 5f) } + @Test + fun `different activities have separate counts - even when called out of order`() { + val sut = fixture.getSut() + val activityAStartFrameCounts = SparseIntArray().also { + it.put(16, 100) + it.put(17, 3) + it.put(700, 2) + it.put(701, 6) + }.let { arrayOf(it) } + val activityBStartFrameCounts = SparseIntArray().also { + it.put(16, 110) + it.put(17, 3) + it.put(700, 3) + it.put(701, 7) + }.let { arrayOf(it) } + val activityAEndFrameCounts = SparseIntArray().also { + it.put(16, 115) + it.put(17, 3) + it.put(700, 5) + it.put(701, 9) + }.let { arrayOf(it) } + val activityBEndFrameCounts = SparseIntArray().also { + it.put(16, 135) + it.put(17, 3) + it.put(700, 8) + it.put(701, 12) + }.let { arrayOf(it) } + + val activityA = fixture.activity + val activityB = mock() + val sentryIdA = fixture.sentryId + val sentryIdB = SentryId() + + whenever(fixture.aggregator.metrics).thenReturn(activityAStartFrameCounts, activityBStartFrameCounts, activityAEndFrameCounts, activityBEndFrameCounts) + + sut.addActivity(activityA) + sut.addActivity(activityB) + sut.setMetrics(activityA, sentryIdA) + sut.setMetrics(activityB, sentryIdB) + + val metricsA = sut.takeMetrics(sentryIdA)!! + val metricsB = sut.takeMetrics(sentryIdB)!! + + val totalFramesA = metricsA!!["frames_total"] + assertEquals(totalFramesA!!.value, 21f) // 15 + 3 + 3 (diff counts for activityA) + + val frozenFramesA = metricsA["frames_frozen"] + assertEquals(frozenFramesA!!.value, 3f) + + val slowFramesA = metricsA["frames_slow"] + assertEquals(slowFramesA!!.value, 3f) + + val totalFramesB = metricsB!!["frames_total"] + assertEquals(totalFramesB!!.value, 35f) // 25 + 5 + 5 (diff counts for activityB) + + val frozenFramesB = metricsB["frames_frozen"] + assertEquals(frozenFramesB!!.value, 5f) + + val slowFramesB = metricsB["frames_slow"] + assertEquals(slowFramesB!!.value, 5f) + } + + @Test + fun `same activity can be used again later on`() { + val sut = fixture.getSut() + val firstLaunchStartFrameCounts = SparseIntArray().also { + it.put(16, 100) + it.put(20, 5) + it.put(701, 6) + }.let { arrayOf(it) } + val firstLaunchEndFrameCounts = SparseIntArray().also { + it.put(16, 110) + it.put(20, 6) + it.put(701, 7) + }.let { arrayOf(it) } + val secondLaunchStartFrameCounts = SparseIntArray().also { + it.put(16, 115) + it.put(20, 8) + it.put(701, 9) + }.let { arrayOf(it) } + val secondLaunchEndFrameCounts = SparseIntArray().also { + it.put(16, 135) + it.put(20, 11) + it.put(701, 12) + }.let { arrayOf(it) } + val secondSentryId = SentryId() + + whenever(fixture.aggregator.metrics).thenReturn(firstLaunchStartFrameCounts, firstLaunchEndFrameCounts, secondLaunchStartFrameCounts, secondLaunchEndFrameCounts) + + sut.addActivity(fixture.activity) + sut.setMetrics(fixture.activity, fixture.sentryId) + sut.addActivity(fixture.activity) + sut.setMetrics(fixture.activity, secondSentryId) + + val metrics = sut.takeMetrics(fixture.sentryId) + val secondMetrics = sut.takeMetrics(secondSentryId) + + val totalFrames = metrics!!["frames_total"] + assertEquals(totalFrames!!.value, 12f) // 10 + 1 + 1 (diff counts for first invocation) + + val frozenFrames = metrics["frames_frozen"] + assertEquals(frozenFrames!!.value, 1f) + + val slowFrames = metrics["frames_slow"] + assertEquals(slowFrames!!.value, 1f) + + val totalFramesSecond = secondMetrics!!["frames_total"] + assertEquals(totalFramesSecond!!.value, 26f) // 20 + 3 + 3 (diff counts for second invocation) + + val frozenFramesSecond = secondMetrics["frames_frozen"] + assertEquals(frozenFramesSecond!!.value, 3f) + + val slowFramesSecond = secondMetrics["frames_slow"] + assertEquals(slowFramesSecond!!.value, 3f) + } + @Test fun `do not set metrics if values are zeroes`() { val sut = fixture.getSut() @@ -109,8 +259,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 +299,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 +324,15 @@ class ActivityFramesTrackerTest { sut.stop() } + @Test + fun `stop resets frameMetricsAggregator`() { + val sut = fixture.getSut() + + sut.stop() + + verify(fixture.aggregator).reset() + } + @Test fun `takeMetrics returns null if no AndroidX`() { whenever(fixture.loadClass.isClassAvailable(any(), any())).thenReturn(false)