Feat: App start up and slow/frozen frames metrics by marandaneto · Pull Request #1466 · getsentry/sentry-java

marandaneto · 2021-05-07T13:23:07Z

📜 Description

Just a POC

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

sentry-android-core/build.gradle.kts

marandaneto · 2021-05-19T08:52:35Z

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

+  //  private boolean isHardwareAccelerated(Activity activity) {
+  //    // we can't observe frame rates for a non hardware accelerated view
+  //    return activity.getWindow() != null
+  //        && ((activity.getWindow().getAttributes().flags
+  //                & WindowManager.LayoutParams.FLAG_HARDWARE_ACCELERATED)
+  //            != 0);
+  //  }


I read that reading slow and frozen frames are only possible if FLAG_HARDWARE_ACCELERATED is enabled, but it worked with it disabled during tests, so I'm not sure if I should keep this check or not.
FLAG_HARDWARE_ACCELERATED is disabled by default by the way

Did you try it in a release build?

yes, maybe thats the case for some OS versions only, I could not find anything though

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

marandaneto · 2021-05-19T08:54:53Z

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

+    final Map<String, @NotNull MeasurementValue> framesMetrics =
+        ActivityFramesState.getInstance().getMetrics();
+    if (framesMetrics != null) {
+      transaction.getMeasurements().putAll(framesMetrics);


slow/frozen frames are always sent, to every transaction, and represents the overall amount of the app lifecycle, and not from the start-end time of the transaction, we can't track only during the start/end of the transaction unless we implement the whole counting ourselves and this changes based on OS version and so on, that's the abstraction the class FrameMetricsAggregator does

Whenever we start a transaction, we could check the current count of the frames in ActivityFramesState. When you finish a transaction, you again call ActivityFramesState, and then you can calculate the different types of frames during the time of the transaction.

that would not scale well if I have multiple transactions at the same time, the same issue as counting on iOS the frames on iOS, we can do it next maybe?

Running into a similar issue on RN as well. Considered @philipphofmann's suggestion at first but on JS users can change the start/end timestamps which makes it not work and way more difficult to implement metrics at the start/end of the transaction.

Why do we run into scaling problems? Hmm, changing it afterward might cause problems. Having it different on platforms is also not a good idea. The advantage of always sending the total number of frames during the lifetime of an application gives you more insights on how many slow frames and frozen frames you have, but the downside is that you have no clue where they are coming from. Being able to correlate them with transactions gives you way more insight. Maybe it would even make sense to send both lifetime frames and frames during the transaction.

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

marandaneto · 2021-05-19T09:22:00Z

@philipphofmann and @bruno-garcia can you give some early feedback? I have left a few comments and TODO with my findings as well

codecov-commenter · 2021-05-19T09:26:49Z

Codecov Report

Merging #1466 (1715c4d) into main (657706a) will decrease coverage by 0.04%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##               main    #1466      +/-   ##
============================================
- Coverage     75.75%   75.71%   -0.05%     
  Complexity     1926     1926              
============================================
  Files           190      191       +1     
  Lines          6580     6585       +5     
  Branches        665      665              
============================================
+ Hits           4985     4986       +1     
- Misses         1270     1274       +4     
  Partials        325      325

Impacted Files	Coverage Δ	Complexity Δ
...main/java/io/sentry/protocol/MeasurementValue.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ain/java/io/sentry/protocol/SentryTransaction.java	`86.20% <50.00%> (-2.69%)`	`12.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 657706a...1715c4d. Read the comment docs.

philipphofmann

Good job for a draft PR. Although it's a draft PR, I added many comments. I guess you would anyway fix most of them before making this PR ready for review. Still, I added them to not forget about them.

philipphofmann · 2021-05-19T12:15:56Z

sentry-android-core/src/main/AndroidManifest.xml

+    <provider
+        android:name=".SentryPerformanceProvider"
+        android:authorities="${applicationId}.SentryPerformanceProvider"
+        android:initOrder="200"


😁. No just kidding.

Suggested change

android:initOrder="200"

android:initOrder="1"

the higher the more important, so it'd make sense to put Long.MAX_VALUE :D

philipphofmann · 2021-05-19T12:21:42Z

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

+            // frozen frames, threshold is 700ms
+            frozenFrames += numFrames;
+          }
+          if (frameTime > 16) {


m: With 60 FPS a frame actually can take 1000 / 60 = 16.66667 ms to render. On Cocoa, I had to change it to 1000 / 59, because almost every frame takes a few nanoseconds longer than the optimal duration. Is there a way to retrieve the current frame rate? It could also be 120 or 90.

I don't think it changes on Android, it's written in their docs and source code in the same way

Interesting. How does it work then if you have a phone with 120 fps?

afaik Android displays are limited to 60 fps

sentry/src/main/java/io/sentry/protocol/SentryTransaction.java

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

philipphofmann · 2021-05-20T07:25:24Z

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

+    return appStartTime;
+  }
+
+  synchronized void setAppStartTime() {


m: We could use Double-Checked Locking to avoid acquiring unnecessary locks. It might make sense to add a comment on why we need synchronization. I guess because someone could initialize the SDK on a background thread?

philipphofmann · 2021-05-20T08:19:23Z

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

+  //  private boolean isHardwareAccelerated(Activity activity) {
+  //    // we can't observe frame rates for a non hardware accelerated view
+  //    return activity.getWindow() != null
+  //        && ((activity.getWindow().getAttributes().flags
+  //                & WindowManager.LayoutParams.FLAG_HARDWARE_ACCELERATED)
+  //            != 0);
+  //  }


Did you try it in a release build?

philipphofmann · 2021-05-20T08:21:13Z

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

+    final Map<String, @NotNull MeasurementValue> framesMetrics =
+        ActivityFramesState.getInstance().getMetrics();
+    if (framesMetrics != null) {
+      transaction.getMeasurements().putAll(framesMetrics);


Whenever we start a transaction, we could check the current count of the frames in ActivityFramesState. When you finish a transaction, you again call ActivityFramesState, and then you can calculate the different types of frames during the time of the transaction.

philipphofmann · 2021-05-20T08:23:59Z

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

+      // this is called after the Activity is created, so we know if the App is a warm or cold
+      // start.


l: Can you maybe elaborate a bit on why this is called after the Activity is created? Why don't we use onActivityCreated instead?

I will add a better comment, but it's hard to explain and requires understanding deeply how the whole process start work, its a few pages that cant be easily resumed in one liner

Can we maybe link to resources so you can find out on your own even when you have to read some articles?

philipphofmann · 2021-05-20T08:25:30Z

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


  @Override
  public synchronized void onActivityStarted(final @NonNull Activity activity) {
+    if (performanceEnabled) {


l: Why don't we do this in onActivityCreated?

because that's when the FrameMetricsAggregator is able to start counting, before that, it can't do anything

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

bruno-garcia · 2021-05-07T20:42:54Z

sentry-android-core/src/main/AndroidManifest.xml

+    <provider
+        android:name=".SentryPerformanceProvider"
+        android:authorities="${applicationId}.SentryInitProvider"
+        android:initOrder="200"


Suggested change

android:initOrder="200"

android:initOrder="1"

😄

see https://github.com/getsentry/sentry-java/pull/1466/files#r635893582

bruno-garcia · 2021-05-07T20:43:32Z

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


  private boolean isAllActivityCallbacksAvailable;

+  // does it need to be atomic? its only in the main thread


is it read by another thread? If no other thread uses it than we're probably fine

bruno-garcia · 2021-05-28T04:25:26Z

sentry/src/main/java/io/sentry/protocol/MeasurementValue.java

+import org.jetbrains.annotations.ApiStatus;
+
+@ApiStatus.Internal
+public final class MeasurementValue {


I wonder if we're better off without this class and using the float directly to avoid allocating these objects

could be easily removed, its not a public API, but the idea was to be extendable like the API is, its an object with a single float property

bruno-garcia · 2021-06-01T20:22:18Z

Is this blocked? Is there anything I can do to help move this PR (particularly the App Startup bits)

marandaneto · 2021-06-02T06:11:34Z

Is this blocked? Is there anything I can do to help move this PR (particularly the App Startup bits)

nop, this PR was the PoC, the PR for App startup is #1487
Next week I'll be focused only on that to make the discussed changes and land it.

marandaneto added 6 commits May 7, 2021 15:22

FeaT: App start up

43c05e5

draft

5bf4f85

remove class

70634ef

draft

9d6a313

Merge branch 'main' into feat/app-start-up

4f249ed

ref

2119d96

marandaneto commented May 18, 2021

View reviewed changes

sentry-android-core/build.gradle.kts Show resolved Hide resolved

fix

dcee890

marandaneto changed the title ~~FeaT: App start up~~ Feat: App start up and slow/frozen frames metrics May 18, 2021

marandaneto added 2 commits May 18, 2021 16:10

ref

163ea22

changelog

38d273b

marandaneto commented May 19, 2021

View reviewed changes

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

marandaneto commented May 19, 2021

View reviewed changes

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

marandaneto added 2 commits May 19, 2021 11:09

fix

34ef096

comment

1715c4d

marandaneto requested review from bruno-garcia and philipphofmann May 19, 2021 09:21

philipphofmann reviewed May 20, 2021

View reviewed changes

draft

9cf5df8

marandaneto mentioned this pull request May 25, 2021

Feat: Measure app start time #1487

Merged

4 tasks

bruno-garcia reviewed May 28, 2021

View reviewed changes

marandaneto mentioned this pull request Jul 19, 2021

Feat: Slow/Frozen frames metrics #1609

Merged

4 tasks

marandaneto closed this in #1609 Jul 28, 2021

marandaneto deleted the feat/app-start-up branch November 23, 2021 08:38

		// this is called after the Activity is created, so we know if the App is a warm or cold
		// start.


		private boolean isAllActivityCallbacksAvailable;

		// does it need to be atomic? its only in the main thread

Uh oh!

Conversation

marandaneto commented May 7, 2021

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marandaneto May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marandaneto commented May 19, 2021

Uh oh!

codecov-commenter commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto May 19, 2021 •

edited

Loading

codecov-commenter commented May 19, 2021 •

edited

Loading