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

Reset frames tracker on activity started #2269

Closed
wants to merge 3 commits into from
Closed

Reset frames tracker on activity started #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70f8e98
Reset frames tracker on activity started
adinauer Sep 30, 2022
c69d5b0
Add test for resetting frames
adinauer Sep 30, 2022
5348768
Add changelog
adinauer Sep 30, 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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Reset frames tracker on activity started ([#2269](https://github.com/getsentry/sentry-java/pull/2269))([#2270](https://github.com/getsentry/sentry-java/pull/2270))

## 6.5.0-beta.2

### Features
Expand Down
1 change: 1 addition & 0 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 @@ -54,6 +54,7 @@ public synchronized void addActivity(final @NotNull Activity activity) {
if (!isFrameMetricsAggregatorAvailable()) {
return;
}
frameMetricsAggregator.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible we have one activity alive with a transaction happening at the time a new one kicks off? Where the reset breaks reporting the values for the first activity?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know you can only have one activity at a time. So for transactions that use ActivityFramesTracker there should not be more than one. But maybe @romtsn or @marandaneto know better.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think this could be a problem if users disable auto-finish of the activity transactions and finish them manually in onStop. Because when you launch a second Activity, the lifecycle will be like this:

ActivityA->onPause
ActivityB->onCreate
ActivityB->onStart
ActivityB->onResume
ActivityA->onStop

so in this case when the ActivityA transaction gets finished in onStop it will have no metrics (or maybe even the wrong metrics from the ActivityB)

Needs to be tested though, just my assumption based on https://developer.android.com/guide/components/activities/activity-lifecycle#coordinating-activities

Copy link
Member Author

@adinauer adinauer Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm need to think some more here. Our other solution was to do the reset after retrieving the counts.

Then this would also cause issues:

ActivityA->onPause
ActivityB->onCreate
ActivityB->onStart [frames should start counting here]
ActivityB->onResume
ActivityA->onStop [reset happens; losing counts from ActivityB as well]

So counts wouldn't be accurate.

Yet another approach was to take snapshots

ActivityA->onPause
ActivityB->onCreate [start snapshot for ActivityB happens here]
ActivityB->onStart
ActivityB->onResume
ActivityA->onStop [stop snapshot for ActivityA happens here]

When can we call reset here and not lose any 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.

Maybe this needs to be a multi-step collection process so we get to call reset somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

After some quick testing:

It stores counts in a SparseIntArray thus only adding an entry for each ms duration value that any frame took (including a count of many frames fell in that time range). As time goes by this means we'll probably have every possible ms length value in the array. For very long transactions (e.g. if a user stayed on a screen for a very long time) and long usage periods on the app this might actually cause problems as we'll have more and more values in the array. So this alone doesn't feel like the right approach to me. Ideally I'd like to get a reset in there somewhere. That's why I suggested we break the tracking apart. Question is if this is worth the effort to invest here if we should rather go with a different appraoch based on Choreographer which we could then also use to track UI events etc.

Copy link
Member

@romtsn romtsn Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yeah feels like custom framecallback approach is the most viable here. We could also persist intermediate counts for ActivityB, so they don't get lost after calling reset, and then sum them up with the new counts after the reset, but if the Choreographer approach is a silver bullet for them all, I'd go for it probably

Copy link
Member Author

Choose a reason for hiding this comment

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

After having a call about this we decided to move forward with calculating the delta value and keep using a single frameMetricsAggregator. If that causes problems we can either use one frameMetricsAggregator per transaction or Activity or investigate e.g. jank stats.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that calculating the delta with a single frameMetricsAggregator should be fine. The distribution of frames should be around 16 ms. I don't expect the SparseIntArray to get too big. Even if we have 1000 different frame durations, which is highly unlikely, we only need 1000 * 32 * 2 bits ~ 8 KiB. We never called reset until now, and it seemed not to cause a memory issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@philipphofmann yeah the case I was worried about was more the one where it tracks until onActivityStopped which might me significantly longer.

frameMetricsAggregator.add(activity);
}

Expand Down
11 changes: 11 additions & 0 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 @@ -126,6 +128,15 @@ class ActivityFramesTrackerTest {
sut.addActivity(fixture.activity)
}

@Test
fun `addActivity resets aggregator`() {
val sut = fixture.getSut()

sut.addActivity(fixture.activity)

verify(fixture.aggregator, times(1)).reset()
}

@Test
fun `setMetrics does not throw if no AndroidX`() {
whenever(fixture.loadClass.isClassAvailable(any(), any<ILogger>())).thenReturn(false)
Expand Down