Add option to enable or disable Activity Frame Tracking #2314

markushi · 2022-10-20T06:18:43Z

📜 Description

Adds an API to enable (that's the default, as before) or disable the Activity Frames Tracking, which is used to report slow and frozen frames.

💡 Motivation and Context

The Activity Frames Tracking has already caused some troubles in the past, and as we depend on androidx it would be great to enable or disable it on demand. Also we recently saw crashes for Android 7.1 (in a very specific setup) caused by using the underlying FrameMetricsAggregator API.

💚 How did you test it?

Added relevant 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

markushi · 2022-10-20T06:19:08Z

Relevant Android 7.1 crash: #2310

github-actions · 2022-10-20T06:19:51Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 21c0739

github-actions · 2022-10-20T06:33:23Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	341.34 ms	379.40 ms	38.06 ms
Size	1.73 MiB	2.32 MiB	608.64 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`4dd88fe`	302.12 ms	331.17 ms	29.04 ms
`649f171`	310.18 ms	357.49 ms	47.31 ms
`2c5f172`	351.18 ms	373.52 ms	22.34 ms
`3d89dea`	345.59 ms	364.06 ms	18.47 ms
`54cebc8`	331.12 ms	385.14 ms	54.02 ms
`c5ccd8a`	329.98 ms	365.52 ms	35.54 ms
`1e4690d`	354.69 ms	387.88 ms	33.19 ms
`7300956`	324.20 ms	353.79 ms	29.58 ms
`649f171`	300.58 ms	367.44 ms	66.86 ms
`3d89dea`	322.38 ms	350.82 ms	28.45 ms

App size

Revision	Plain	With Sentry	Diff
`4dd88fe`	1.73 MiB	2.29 MiB	579.50 KiB
`649f171`	1.73 MiB	2.32 MiB	608.44 KiB
`2c5f172`	1.73 MiB	2.29 MiB	580.10 KiB
`3d89dea`	1.74 MiB	2.33 MiB	604.92 KiB
`54cebc8`	1.73 MiB	2.29 MiB	579.43 KiB
`c5ccd8a`	1.74 MiB	2.33 MiB	607.44 KiB
`1e4690d`	1.74 MiB	2.33 MiB	604.92 KiB
`7300956`	1.73 MiB	2.29 MiB	578.69 KiB
`649f171`	1.73 MiB	2.32 MiB	608.44 KiB
`3d89dea`	1.74 MiB	2.33 MiB	604.92 KiB

Previous results on branch: feat/activity-frames-tracker-feature-flag

Startup times

Revision	Plain	With Sentry	Diff
`1fa1cb9`	315.24 ms	353.04 ms	37.80 ms
`ab76c2b`	308.43 ms	342.78 ms	34.35 ms
`217d03a`	302.60 ms	354.96 ms	52.36 ms
`a3665da`	379.18 ms	470.32 ms	91.14 ms
`0052e93`	297.56 ms	317.12 ms	19.56 ms
`6375221`	302.48 ms	336.60 ms	34.12 ms

App size

Revision	Plain	With Sentry	Diff
`1fa1cb9`	1.73 MiB	2.32 MiB	607.13 KiB
`ab76c2b`	1.73 MiB	2.32 MiB	608.62 KiB
`217d03a`	1.73 MiB	2.32 MiB	608.62 KiB
`a3665da`	1.73 MiB	2.32 MiB	607.13 KiB
`0052e93`	1.73 MiB	2.32 MiB	607.15 KiB
`6375221`	1.73 MiB	2.32 MiB	607.14 KiB

codecov-commenter · 2022-10-20T06:37:58Z

Codecov Report

Base: 80.21% // Head: 80.21% // No change to project coverage 👍

Coverage data is based on head (fc2948e) compared to base (649f171).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #2314   +/-   ##
=========================================
  Coverage     80.21%   80.21%           
  Complexity     3475     3475           
=========================================
  Files           247      247           
  Lines         12906    12906           
  Branches       1735     1735           
=========================================
  Hits          10352    10352           
  Misses         1894     1894           
  Partials        660      660

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.

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

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

romtsn

LGTM, besides a few things 👍

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

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

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

marandaneto · 2022-10-20T07:46:05Z

@markushi have you tried using an Android emulator 7.2.1?
Opt out is a good option but I'm also interested in the root cause.

adinauer

LGTM only one question which @romtsn may have a stronger opinion about.

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

philipphofmann

A question about the naming of the option.

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

markushi · 2022-10-20T08:59:14Z

@markushi have you tried using an Android emulator 7.2.1? Opt out is a good option but I'm also interested in the root cause.

I've tried it on 7.1.1 (API 25) and the next available one: 8.0 (API 26), as well as 30, 31 and 33. Starting with 26 the issues does not occur anymore. The relevant error seems to happen on the native Android side when collecting the FrameMetrics, so there's little we can do about it. Here's the relevant snippet from the logs

JNI ERROR (app bug): local reference table overflow (max=512)
local reference table dump:
Last 10 entries (of 512):
    511: 0x2aef4790 long[] (16 elements)
    510: 0x2aecb110 android.view.FrameMetrics
    509: 0x2aef4ca0 long[] (16 elements)
    508: 0x2aecb880 android.view.FrameMetrics
    507: 0x2aede310 long[] (16 elements)
    506: 0x2aebe080 android.view.FrameMetrics
    505: 0x2aede8b0 long[] (16 elements)
    504: 0x2aebe8b0 android.view.FrameMetrics
    503: 0x2aedee50 long[] (16 elements)
    502: 0x2aeadd70 android.view.FrameMetrics

marandaneto · 2022-10-20T09:54:42Z

@markushi have you tried using an Android emulator 7.2.1? Opt out is a good option but I'm also interested in the root cause.

I've tried it on 7.1.1 (API 25) and the next available one: 8.0 (API 26), as well as 30, 31 and 33. Starting with 26 the issues does not occur anymore. The relevant error seems to happen on the native Android side when collecting the FrameMetrics, so there's little we can do about it. Here's the relevant snipped from the logs
JNI ERROR (app bug): local reference table overflow (max=512)
local reference table dump:
Last 10 entries (of 512):
    511: 0x2aef4790 long[] (16 elements)
    510: 0x2aecb110 android.view.FrameMetrics
    509: 0x2aef4ca0 long[] (16 elements)
    508: 0x2aecb880 android.view.FrameMetrics
    507: 0x2aede310 long[] (16 elements)
    506: 0x2aebe080 android.view.FrameMetrics
    505: 0x2aede8b0 long[] (16 elements)
    504: 0x2aebe8b0 android.view.FrameMetrics
    503: 0x2aedee50 long[] (16 elements)
    502: 0x2aeadd70 android.view.FrameMetrics

try to bump the androidx.core for testing? maybe its something fixed in there?

markushi · 2022-10-20T13:08:45Z

try to bump the androidx.core for testing? maybe its something fixed in there?

@marandaneto I tried with the latest available version(androidx.core:core:1.9.0) - still the same issue 😬

If anyone ever runs in the same issue again, I've used the following to automate turning on/off the screen many times:

while true; do adb shell input keyevent 26 && sleep 1; done

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

marandaneto · 2022-10-24T07:20:52Z

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

  public ActivityFramesTracker(final @NotNull LoadClass loadClass) {
-    this(loadClass, null);
+    this(loadClass, new SentryAndroidOptions());


Creating a new options here won't help, the feature will be always enabled and can't be disabled since options is private, maybe better to just remove this ctor in this case and fix on Flutter/RN if necessary.

the feature will be always enabled

exactly, that matches the existing behavior thus I thought it makes sense. But in hindsight it's actually more confusing than helpful, let me remove this again 👍

marandaneto · 2022-10-24T07:21:06Z

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

@@ -25,48 +25,57 @@
 public final class ActivityFramesTracker {

  private @Nullable FrameMetricsAggregator frameMetricsAggregator = null;
-  private boolean androidXAvailable = true;
+  private final SentryAndroidOptions options;


Missing nullability anotations.

Add option to enable or disable Activity Frames Tracker

fb45b6d

markushi requested review from adinauer, romtsn and stefanosiano as code owners October 20, 2022 06:18

markushi mentioned this pull request Oct 20, 2022

Crash on Android 7.1.2 #2310

Closed

Add Activity Frames Tracker to changelog

fd99cb6

markushi commented Oct 20, 2022

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java Outdated Show resolved Hide resolved

marandaneto reviewed Oct 20, 2022

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/IActivityFramesTracker.java Outdated Show resolved Hide resolved

romtsn reviewed Oct 20, 2022

View reviewed changes

adinauer approved these changes Oct 20, 2022

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java Outdated Show resolved Hide resolved

Merge branch 'main' into feat/activity-frames-tracker-feature-flag

e48e527

philipphofmann reviewed Oct 20, 2022

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java Outdated Show resolved Hide resolved

Use options flag instead of Noop impl for FrameTracker, improve naming

88fe044

markushi changed the title ~~Add option to enable or disable Activity Frames Tracker~~ Add option to enable or disable Activity Frame Tracking Oct 20, 2022

Merge branch 'main' into feat/activity-frames-tracker-feature-flag

f7368b0

romtsn reviewed Oct 21, 2022

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Show resolved Hide resolved

romtsn approved these changes Oct 21, 2022

View reviewed changes

Restore ActivityFramesTracker constructor, as it's required for flutter

afb587e

markushi mentioned this pull request Oct 24, 2022

Use new constructor which respects Frame Tracking feature flag getsentry/sentry-dart#1087

Merged

7 tasks

Fix missing @deprecated annotation for ActivityFramesTracker

fb706a8

marandaneto reviewed Oct 24, 2022

View reviewed changes

markushi added 3 commits October 24, 2022 09:26

Fix disable InlineMeSuggester error

0ebcc82

Add missing .api file update

fc2948e

Adapt ActivityFramesTracker ctor and options nullability

21c0739

markushi merged commit 83be7c7 into main Oct 24, 2022

markushi deleted the feat/activity-frames-tracker-feature-flag branch October 24, 2022 09:59

gslevinkov mentioned this pull request Oct 27, 2022

NO-TICKET - update Sentry version to 6.6.0 p2p-org/key-app-android#924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to enable or disable Activity Frame Tracking #2314

Add option to enable or disable Activity Frame Tracking #2314

markushi commented Oct 20, 2022

markushi commented Oct 20, 2022

github-actions bot commented Oct 20, 2022 •

edited

github-actions bot commented Oct 20, 2022 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/activity-frames-tracker-feature-flag

Startup times

App size

codecov-commenter commented Oct 20, 2022 •

edited

romtsn left a comment

marandaneto commented Oct 20, 2022

adinauer left a comment

philipphofmann left a comment

markushi commented Oct 20, 2022 •

edited

marandaneto commented Oct 20, 2022

markushi commented Oct 20, 2022

marandaneto Oct 24, 2022

markushi Oct 24, 2022

marandaneto Oct 24, 2022

Add option to enable or disable Activity Frame Tracking #2314

Add option to enable or disable Activity Frame Tracking #2314

Conversation

markushi commented Oct 20, 2022

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

markushi commented Oct 20, 2022

github-actions bot commented Oct 20, 2022 • edited

github-actions bot commented Oct 20, 2022 • edited

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/activity-frames-tracker-feature-flag

Startup times

App size

codecov-commenter commented Oct 20, 2022 • edited

Codecov Report

romtsn left a comment

Choose a reason for hiding this comment

marandaneto commented Oct 20, 2022

adinauer left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

markushi commented Oct 20, 2022 • edited

marandaneto commented Oct 20, 2022

markushi commented Oct 20, 2022

marandaneto Oct 24, 2022

Choose a reason for hiding this comment

markushi Oct 24, 2022

Choose a reason for hiding this comment

marandaneto Oct 24, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 20, 2022 •

edited

github-actions bot commented Oct 20, 2022 •

edited

codecov-commenter commented Oct 20, 2022 •

edited

markushi commented Oct 20, 2022 •

edited