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.

Sign up for GitHub

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

Fix slow and frozen frames tracking #2271

Merged
merged 8 commits into from
Oct 5, 2022
+286 −39
Merged

Fix slow and frozen frames tracking #2271

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
343ada1
Snapshot frames onActivityStarted and on transaction finish then calc…
adinauer Oct 3, 2022
6044b4c
Add changelog
adinauer Oct 3, 2022
2f4462c
Add tests
adinauer Oct 3, 2022
f6b0dc7
Add final keyword
adinauer Oct 3, 2022
4f2ca28
Update sentry-android-core/src/main/java/io/sentry/android/core/Activ…
adinauer Oct 4, 2022
8cab3eb
Update sentry-android-core/src/test/java/io/sentry/android/core/Activ…
adinauer Oct 4, 2022
391ea57
Update sentry-android-core/src/test/java/io/sentry/android/core/Activ…
adinauer Oct 4, 2022
5b7f10b
Code Review changes
adinauer Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## Unreleased

- Fix slow and frozen frames tracking ([#2271](https://github.com/getsentry/sentry-java/pull/2271))
Copy link
Member Author

Choose a reason for hiding this comment

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

@marandaneto since no public API changes were necessary I assume this fix doesn't require any further action on hybrid SDKs.


## 6.4.2

### Fixes
Expand Down
105 changes: 83 additions & 22 deletions sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,6 +28,8 @@ public final class ActivityFramesTracker {

private final @NotNull Map<SentryId, Map<String, @NotNull MeasurementValue>>
activityMeasurements = new ConcurrentHashMap<>();
private final @NotNull Map<Activity, FrameCounts> frameCountAtStartSnapshots =
Copy link
Member

Choose a reason for hiding this comment

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

I like the name 👏

new WeakHashMap<>();

public ActivityFramesTracker(final @NotNull LoadClass loadClass, final @Nullable ILogger logger) {
androidXAvailable =
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

l: I would move this method below setMetrics, because it is the only function calling this method. Now when I read setMetrics I have to jump all the way up to here to find the method. Keeping functions calling each other close to each other in the code improves readability, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, forgot to reorder them

@Nullable
final FrameCounts frameCountsAtStartFromMap = frameCountAtStartSnapshots.remove(activity);
@NotNull
final FrameCounts frameCountsAtStart =
frameCountsAtStartFromMap == null ? new FrameCounts(0, 0, 0) : frameCountsAtStartFromMap;
Copy link
Member

Choose a reason for hiding this comment

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

m: When do we run into this edge case that the frameCountsAtStartFromMap is null? We already check that the activity is not null here

sentry-java/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

Lines 169 to 172 in f6b0dc7

@Nullable Activity unwrappedActivity = weakActivity.get();
if (unwrappedActivity != null) {
activityFramesTracker.setMetrics(
unwrappedActivity, finishingTransaction.getEventId());

The parameter is also annotated @NotNull. If this happens, something is wrong and I think we should better not count the frames at all and return null here. If we use FrameCounts(0, 0, 0), I think we risk calculating incorrect frame counts.

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea here was that we might not be able to determine frame counts if the FrameMetricsAggregator was freshly created as getMetrics() on it is marked with @Nullable. Since I changed some code around it shouldn't be necessary as we have a FrameCounts(0, 0, 0). Gonna change it so it doesn't calculate a difference if it can't find a start snapshot.


@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++) {
Expand All @@ -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) {
adinauer marked this conversation as resolved.
Show resolved Hide resolved
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<String, @NotNull MeasurementValue> measurements = new HashMap<>();
measurements.put("frames_total", tfValues);
measurements.put("frames_slow", sfValues);
Expand All @@ -132,6 +192,7 @@ public synchronized void setMetrics(
public synchronized void stop() {
if (isFrameMetricsAggregatorAvailable()) {
frameMetricsAggregator.stop();
frameMetricsAggregator.reset();
}
activityMeasurements.clear();
}
Expand Down
83 changes: 75 additions & 8 deletions sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

l:

Suggested change
val arrayAll = SparseIntArray()
arrayAll.put(1, 100)
arrayAll.put(20, 5)
arrayAll.put(705, 6)
val arrayAll = SparseIntArray().apply {
put(1, 100)
put(20, 5)
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)
Expand All @@ -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)
Expand All @@ -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<ILogger>())).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)
Expand All @@ -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()
adinauer marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
fun `takeMetrics returns null if no AndroidX`() {
whenever(fixture.loadClass.isClassAvailable(any(), any<ILogger>())).thenReturn(false)
Expand Down