RUMM-3216 use jank stats by xgouchet · Pull Request #1405 · DataDog/dd-sdk-android

xgouchet · 2023-04-28T07:11:21Z

What does this PR do?

Use the JankStats library to get the actual frame durations.

Motivation

Using the Choreographer like we did forced a redraw of all frames, which was not necessary if the content didn't require it.

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

0xnm

nice solution! I've added few comments to clarify.

0xnm · 2023-04-28T07:52:22Z

+        @Suppress(
+            "UnsafeThirdPartyFu" +
+                "nctionCall"
+        ) // pool size can't be <= 0


seems like a wrong change

0xnm · 2023-04-28T07:54:22Z

+    // region JankStats.OnFrameListener
+
+    override fun onFrame(volatileFrameData: FrameData) {
+        println("$volatileFrameData")


to be removed

0xnm · 2023-04-28T07:58:23Z

+kotlin = "1.8.10"
+kotlinSP = "1.8.10-1.0.9"


do we need it as a part of this PR? I would rather handle it in the SDK v2 branch with Gradle 8 update (which is based on Kotlin 1.8 as well, so that we don't have conflicts for the build classpath), and also checking API/ABI compatibility.

Unfortunately I need to use this version otherwise there's an incompatibility with the Metrics library.

ok, we still have the following anyway in the Kotlin compiler configuration:

apiVersion = "1.6" languageVersion = "1.6"

So shouldn't be any difference on the client side apart of the dependency resolution. It can be though the issue on the client side that Kotlin is resolved to 1.8 and thus produces bytecode not compatible with something else (like React Native), but at least from the SDK integration point should be good.

0xnm · 2023-04-28T07:59:13Z

 import androidx.lifecycle.ViewModelProviders
 import androidx.navigation.NavController
-import androidx.navigation.fragment.NavHostFragment.findNavController
+import androidx.navigation.fragment.NavHostFragment.Companion.findNavController


Why do we need to specify Companion now? Is it related to the Kotlin 1.8 change?

Yes, it's changed because of Kotlin 1.8

mariusc83 · 2023-04-28T11:19:49Z

+
+    override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) {
+        try {
+            JankStats.createAndTrack(activity.window, this)


this will work only for activity windows but will not measure the frame rate for DialogWindow or PopUpWindow. I guess once we add that ticket to support that in RUM we will have to find a different way to plugin in the JankStats into those windows too.

+ upgrade kotlin related dependencies (needed to be compatible with the Metrics library)

0xnm

nice! I've left a comment wondering if we should change also the way we detect refresh rate.

0xnm · 2023-05-03T12:29:42Z

-            )
-        }
+        jankStatsActivityLifecycleListener = JankStatsActivityLifecycleListener(frameRateVitalMonitor, internalLogger)
+        (appContext as? Application)?.registerActivityLifecycleCallbacks(jankStatsActivityLifecycleListener)


should we modify the way we are checking refresh rate? currently we are using RumViewScope#detectRefreshRateScale, which relies on the view key and the presence of activity/fragment there (which may be missing in case of manual instrumentation or calls coming from cross-platform).

But now, since we are listening for activities anyway, we can access UI context at any time and basically pass this info with a sample, making things work properly even for cross-platform.

wdyt?

That makes a lot of sense indeed

0xnm · 2023-05-03T12:31:06Z

+
+    override fun onFrame(volatileFrameData: FrameData) {
+        val durationNs = volatileFrameData.frameDurationUiNanos
+        if (durationNs > 0.0) {


in which case it can be negative or zero?

…istener

codecov-commenter · 2023-05-24T09:16:55Z

Codecov Report

Merging #1405 (f0c40b0) into develop (8586d84) will increase coverage by 0.16%.
The diff coverage is 86.49%.

@@             Coverage Diff             @@
##           develop    #1405      +/-   ##
===========================================
+ Coverage    81.94%   82.09%   +0.16%     
===========================================
  Files          363      363              
  Lines        13031    12974      -57     
  Branches      2191     2164      -27     
===========================================
- Hits         10677    10651      -26     
+ Misses        1688     1668      -20     
+ Partials       666      655      -11

Impacted Files	Coverage Δ
...ernal/vitals/JankStatsActivityLifecycleListener.kt	`80.00% <80.00%> (ø)`
...lin/com/datadog/android/rum/internal/RumFeature.kt	`92.54% <100.00%> (-1.03%)`	⬇️
...d/rum/internal/domain/scope/RumViewManagerScope.kt	`96.15% <100.00%> (+0.77%)`	⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt	`95.80% <100.00%> (+1.48%)`	⬆️

... and 54 files with indirect coverage changes

mariusc83

Good work Xav ;)

xgouchet requested a review from a team as a code owner April 28, 2023 07:11

0xnm reviewed Apr 28, 2023

View reviewed changes

xgouchet changed the title ~~Xgouchet/rumm 3216/jank stats~~ RUMM-3216 use jank stats Apr 28, 2023

mariusc83 reviewed Apr 28, 2023

View reviewed changes

RUMM-3216 add AndroidX Metrics dependency

a0de2fb

+ upgrade kotlin related dependencies (needed to be compatible with the Metrics library)

xgouchet force-pushed the xgouchet/RUMM-3216/jank_stats branch from 82c1637 to 3ccb996 Compare May 3, 2023 10:01

xgouchet added 2 commits May 3, 2023 14:04

RUMM-3216 create JankStats listener

6c3251e

RUMM-3216 use the JankStats listener for frameRate vitals

2ce0d84

xgouchet force-pushed the xgouchet/RUMM-3216/jank_stats branch from 3ccb996 to 2ce0d84 Compare May 3, 2023 12:04

xgouchet requested review from 0xnm and mariusc83 May 3, 2023 12:04

0xnm reviewed May 3, 2023

View reviewed changes

xgouchet added 2 commits May 23, 2023 12:02

RUMM-3216 detect device refresh rate from JankStatsActivityLifecycleL…

0c013ca

…istener

RUMM-3216 remove refresh rate scaling in RumViewScope

c234bdb

xgouchet requested a review from 0xnm May 23, 2023 12:35

0xnm approved these changes May 23, 2023

View reviewed changes

RUMM-3216 fix linting

f0c40b0

mariusc83 approved these changes May 24, 2023

View reviewed changes

xgouchet merged commit e2074ea into develop May 24, 2023

xgouchet deleted the xgouchet/RUMM-3216/jank_stats branch May 24, 2023 12:18

xgouchet added this to the 2.0.0 milestone Dec 13, 2023

Uh oh!

Conversation

xgouchet commented Apr 28, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

0xnm 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!

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!

0xnm 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!

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

Uh oh!

Codecov Report

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 24, 2023 •

edited

Loading