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

Jump to bottom

Fix slow and frozen frames tracking #2271

Merged
merged 8 commits into from Oct 5, 2022
Merged

Fix slow and frozen frames tracking #2271

merged 8 commits into from Oct 5, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Oct 3, 2022 •
edited

📜 Description

Fixes the wrong reporting of frames by snapshotting slow and frozen frame values at the start and end, then calculating a delta.

💡 Motivation and Context

Fixes #2264

💚 How did you test it?

Sample App; Unit Tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

Once merged, release 6.4.3 then merge 6.4.x into main.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022 •
edited

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 274.33 ms 305.76 ms 31.43 ms
Size 1.74 MiB 2.33 MiB 605.39 KiB

Previous results on branch: fix/6_4_fix_frames_tracking_delta

Startup times

Revision Plain With Sentry Diff
5f9ea6f 304.71 ms 345.49 ms 40.78 ms
001064a 327.52 ms 390.29 ms 62.77 ms
926b4a3 307.96 ms 347.68 ms 39.72 ms

App size

Revision Plain With Sentry Diff
5f9ea6f 1.74 MiB 2.33 MiB 605.44 KiB
001064a 1.74 MiB 2.33 MiB 605.44 KiB
926b4a3 1.74 MiB 2.33 MiB 605.44 KiB

@adinauer adinauer mentioned this pull request Oct 3, 2022
Closed
4 tasks
adinauer
adinauer commented Oct 3, 2022
View reviewed changes
CHANGELOG.md
@@ -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.

@adinauer adinauer mentioned this pull request Oct 3, 2022
Closed
4 tasks
@philipphofmann
Copy link
Member

philipphofmann commented Oct 3, 2022 •
edited

I just did a review for this PR and looked at the ActivityLifecycleIntegration to find out how this code works together in combination with it. We thought that we can't call frameMetricsAggregator.reset(), because if users disable auto-finish of the activity transactions and finish them manually in onStop.; see #2269 (comment)

If I'm not mistaken, we can't have multiple auto generated activity transactions running in parallel, because call stopPreviousTransactions in here

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

Lines 152 to 156 in 7300956

private void startTracing(final @NotNull Activity activity) {
WeakReference<Activity> weakActivity = new WeakReference<>(activity);
if (performanceEnabled && !isRunningTransaction(activity) && hub != null) {
// as we allow a single transaction running on the bound Scope, we finish the previous ones
stopPreviousTransactions();

So either we keep the code in ActivityLifecycleIntegration and we can safely call frameMetricsAggregator.reset() or we also need to adapt the code above.

@romtsn
Copy link
Member

romtsn commented Oct 3, 2022

I just did a review for this PR and looked at the ActivityLifecycleIntegration to find out how this code works together in combination with it. We thought that we can't call frameMetricsAggregator.reset(), because if users disable auto-finish of the activity transactions and finish them manually in onStop.; see #2269 (comment)

If I'm not mistaken, we can't have multiple auto generated activity transactions running in parallel, because call stopPreviousTransactions in here

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

Lines 152 to 156 in 7300956

private void startTracing(final @NotNull Activity activity) {
WeakReference<Activity> weakActivity = new WeakReference<>(activity);
if (performanceEnabled && !isRunningTransaction(activity) && hub != null) {
// as we allow a single transaction running on the bound Scope, we finish the previous ones
stopPreviousTransactions();

So either we keep the code in ActivityLifecycleIntegration and we can safely call frameMetricsAggregator.reset() or we also need to adapt the code above.

The problem is if the transaction still has unfinished children, then calling stopPreviousTransaction won't actually finish them until the children are finished. But when the children get finished, we already reset the frame metrics, so the transaction won't have them at all.

@philipphofmann
Copy link
Member

The problem is if the transaction still has unfinished children, then calling stopPreviousTransaction won't actually finish them until the children are finished. But when the children get finished, we already reset the frame metrics, so the transaction won't have them at all.

Ah, I missed that one, thanks for pointing that out.

philipphofmann
philipphofmann reviewed Oct 4, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

One edge case maybe to fix. Almost LGTM. Thanks for fixing this, @adinauer 👏 .

I added plenty of nitpicks; you can ignore them if you want. Just minor things to improve the readability of the code a bit. Some of them could also be personal preference.

sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated Show resolved Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated Show resolved Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated
Comment on lines 148 to 150
it.put(1, 110)
it.put(20, 6)
it.put(705, 7)
Copy link
Member

Choose a reason for hiding this comment

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

m: Testing with boundary values of the equivalence classes [0-16] [17-700] [701 - infinity] makes the tests a bit better. This helps to detect off by 1 errors.

Suggested change
it.put(1, 110)
it.put(20, 6)
it.put(705, 7)
it.put(16, 110)
it.put(17, 3) // Splitted up 20
it.put(700, 3)
it.put(701, 7)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just used the values that were there in old tests; can update

Copy link
Member

Choose a reason for hiding this comment

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

Up to you.

sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated Show resolved Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated
Comment on lines 116 to 119
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)
}

sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
@@ -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 👏

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Outdated
Comment on lines 75 to 76
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.

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Outdated
}
}

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

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Outdated Show resolved Hide resolved
adinauer and others added 4 commits October 4, 2022 11:36
@adinauer @philipphofmann
Update sentry-android-core/src/main/java/io/sentry/android/core/Activ…
4f2ca28 
…ityFramesTracker.java

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@adinauer @philipphofmann
Update sentry-android-core/src/test/java/io/sentry/android/core/Activ…
8cab3eb 
…ityFramesTrackerTest.kt

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@adinauer @philipphofmann
Update sentry-android-core/src/test/java/io/sentry/android/core/Activ…
391ea57 
…ityFramesTrackerTest.kt

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@adinauer
Code Review changes
5b7f10b
philipphofmann
philipphofmann approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thank you @adinauer 👏

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
}

private @Nullable FrameCounts diffFrameCountsAtEnd(final @NotNull Activity activity) {
@Nullable final FrameCounts frameCountsAtStart = frameCountAtStartSnapshots.remove(activity);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks 🙏

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (6.4.x@76d3f4a). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff            @@
##             6.4.x    #2271   +/-   ##
========================================
  Coverage         ?   80.62%           
  Complexity       ?     3368           
========================================
  Files            ?      240           
  Lines            ?    12388           
  Branches         ?     1646           
========================================
  Hits             ?     9988           
  Misses           ?     1791           
  Partials         ?      609

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

romtsn
romtsn approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@adinauer adinauer merged commit 335c7b1 into 6.4.x Oct 5, 2022
@adinauer adinauer deleted the fix/6_4_fix_frames_tracking_delta branch October 5, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipphofmann philipphofmann philipphofmann approved these changes

@romtsn romtsn romtsn approved these changes

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adinauer @philipphofmann @romtsn @codecov-commenter