[Obsolete] Capture frame metrics in fragment monitor #3584

jeremyjiang-dev · 2022-03-23T20:18:54Z

Please review #3592 instead

google-oss-bot · 2022-03-23T20:25:38Z

Coverage Report ¹

Affected Products

No changes between base commit (c08ebb7) and merge commit (a584202).

Test Logs

Merge (a584202): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3584/check-coverage-changed/1507443217799319554

Notes

Commit (a584202) is created by Prow via merging PR base commit (c08ebb7) and head commit (a0e6491).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/zX29si0a3u.html

google-oss-bot · 2022-03-23T20:26:09Z

The public api surface has changed for the subproject firebase-perf:
error: Added class com.google.firebase.perf.metrics.FrameMetricsCalculator [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2022-03-23T20:34:24Z

Size Report ¹

Affected Products

base
Type Base (c08ebb7) Merge (a584202) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-abt
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 14.0 kB ? (?)
apk (aggressive) ? 84.9 kB ? (?)
apk (release) ? 685 kB ? (?)
firebase-annotations
Type Base (c08ebb7) Merge (a584202) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.88 kB ? (?)
firebase-common
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 48.0 kB ? (?)
apk (aggressive) ? 84.2 kB ? (?)
apk (release) ? 680 kB ? (?)
firebase-components
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 41.4 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 29.5 kB ? (?)
firebase-config
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 62.7 kB ? (?)
apk (aggressive) ? 93.3 kB ? (?)
apk (release) ? 731 kB ? (?)
firebase-datatransport
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 5.12 kB ? (?)
apk (aggressive) ? 130 kB ? (?)
apk (release) ? 767 kB ? (?)
firebase-encoders
Type Base (c08ebb7) Merge (a584202) Diff
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 15.3 kB ? (?)
firebase-encoders-json
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 10.5 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 20.1 kB ? (?)
firebase-encoders-proto
Type Base (c08ebb7) Merge (a584202) Diff
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 21.6 kB ? (?)
firebase-installations
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 55.0 kB ? (?)
apk (aggressive) ? 85.8 kB ? (?)
apk (release) ? 702 kB ? (?)
firebase-installations-interop
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 8.34 kB ? (?)
apk (aggressive) ? 65.0 kB ? (?)
apk (release) ? 651 kB ? (?)
firebase-perf
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 306 kB ? (?)
apk (aggressive) ? 1.03 MB ? (?)
apk (release) ? 2.46 MB ? (?)
protolite-well-known-types
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 993 kB ? (?)
apk (aggressive) ? 134 kB ? (?)
apk (release) ? 661 kB ? (?)
transport-api
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 6.57 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 14.9 kB ? (?)
transport-backend-cct
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 53.5 kB ? (?)
apk (aggressive) ? 58.0 kB ? (?)
apk (release) ? 105 kB ? (?)
transport-runtime
Type Base (c08ebb7) Merge (a584202) Diff
aar ? 178 kB ? (?)
apk (aggressive) ? 43.8 kB ? (?)
apk (release) ? 82.6 kB ? (?)

Test Logs

Merge (a584202): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3584/binary-size/1507443217795125248

Notes

Commit (a584202) is created by Prow via merging PR base commit (c08ebb7) and head commit (a0e6491).

https://storage.googleapis.com/firebase-sdk-metric-reports/RFkttN0VYv.html

google-oss-bot · 2022-03-23T20:50:50Z

The public api surface has changed for the subproject firebase-perf:
error: Added class com.google.firebase.perf.metrics.FrameMetricsCalculator [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

firebase-perf/src/test/java/com/google/firebase/perf/application/FragmentStateMonitorTest.java

jeremyjiang-dev · 2022-03-24T23:23:59Z

/test check-changed

visumickey · 2022-03-25T05:03:21Z

The public api surface has changed for the subproject firebase-perf: error: Added class com.google.firebase.perf.metrics.FrameMetricsCalculator [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

Has the public surface changed at all? Can we make sure to have the visibility of APIs rightly set?

visumickey · 2022-03-25T05:33:21Z

firebase-perf/src/main/java/com/google/firebase/perf/application/FragmentStateMonitor.java

    fragmentToTraceMap.put(f, fragmentTrace);
+
+    FrameMetrics frameMetrics =
+        FrameMetricsCalculator.calculateFrameMetrics(this.frameMetricsAggregator.getMetrics());


Since this is the start of the fragment trace, should we calculateFramemetrics here? I would assume that would be empty. If that is true, can we replace this with an empty instance of FrameMetrics?

The frameMetrics will not be empty because we are not resetting the frameMetricsAggregator before fragment starts. The approach here is to take a snapshot of the frame metrics when a fragment starts and another snapshot when a fragment finishes. We will calculate the difference between these two snapshots as the fragment's screen performance.

visumickey · 2022-03-25T05:38:55Z

firebase-perf/src/main/java/com/google/firebase/perf/application/FragmentStateMonitor.java

+    int totalFrames = curFrameMetrics.getTotalFrames() - preFrameMetrics.getTotalFrames();
+    int slowFrames = curFrameMetrics.getSlowFrames() - preFrameMetrics.getSlowFrames();
+    int frozenFrames = curFrameMetrics.getFrozenFrames() - preFrameMetrics.getFrozenFrames();
+
+    if (totalFrames == 0 && slowFrames == 0 && frozenFrames == 0) {
+      // All metrics are zero, no need to send screen trace.
+      return;
+    }
+    // Only putMetric if corresponding metric is non-zero.
+    if (totalFrames > 0) {
+      fragmentTrace.putMetric(Constants.CounterNames.FRAMES_TOTAL.toString(), totalFrames);
+    }
+    if (slowFrames > 0) {
+      fragmentTrace.putMetric(Constants.CounterNames.FRAMES_SLOW.toString(), slowFrames);
+    }
+    if (frozenFrames > 0) {
+      fragmentTrace.putMetric(Constants.CounterNames.FRAMES_FROZEN.toString(), frozenFrames);
+    }


Can we make this a ScreenTrace/TraceUtil method that takes the FrameMetrics as an input and adds all necessary counters internally? This method could also be used in the AppState Monitor for the screen trace.

Can you also update AppStateMonitor to also use the newer utils like FrameMetricsCalculator and ScreenTraceUtils?

Updated in the new PR #3592

visumickey · 2022-03-25T05:45:36Z

firebase-perf/src/main/java/com/google/firebase/perf/metrics/FrameMetricsCalculator.java

+ *
+ * @hide
+ */
+public class FrameMetricsCalculator {


Can you add a package-info.java to qualify the visibility of the package across the SDK?

A sample reference would be https://github.com/firebase/firebase-android-sdk/blob/master/firebase-perf/src/main/java/com/google/firebase/perf/metrics/validator/package-info.java

I cannot add package level visibility to com.google.firebase.perf.metrics since Trace.java needs to be publicly visible.

visumickey · 2022-03-25T05:49:18Z

firebase-perf/src/main/java/com/google/firebase/perf/metrics/FrameMetricsCalculator.java

+    int frozenFrames = 0;
+
+    if (arr != null) {
+      SparseIntArray frameTimes = arr[FrameMetricsAggregator.TOTAL_INDEX];


For learning - Can you clarify what is FrameMetricsAggregator.TOTAL_INDEX? Curious if we should pass the frameMetricsAggregator instance here?

Total index: The index in the metrics array where the data for TOTAL_DURATION is stored. We calculate the slow/frozen frames from the TOTAL_DURATION.

The reason that I did not pass the frameMetricsAggregator instance is that we might get the arr from different methods from frameMetricsAggregator, such as getMetrics(), remove(), or reset(). Let me know if you think passing the instance is better.

firebase-perf/src/test/java/com/google/firebase/perf/application/FragmentStateMonitorTest.java

visumickey · 2022-03-25T05:59:30Z

firebase-perf/src/test/java/com/google/firebase/perf/application/FragmentStateMonitorTest.java

+
+  @Test
+  public void
+      lifecycleCallbacks_differentFrameMetricsCapturedByFma_logFragmentScreenTraceWithCorrectFrames() {


Can we add a testcase where the "onPaused" method of the fragment gets called more than once and ensure that we do not trigger event dispatch more than once?

Also, vicecersa where "onResume" method is not called for a fragment and ensure that we do not break/crash when "onPaused" is called.

Great ideas! Done!

visumickey · 2022-03-25T06:02:46Z

firebase-perf/src/test/java/com/google/firebase/perf/metrics/FrameMetricsCalculatorTest.java

+    // Slow frames have duration greater than 16ms and frozen frames have duration greater than
+    // 700ms. The key value pair means (duration, num_of_samples).
+    SparseIntArray sparseIntArray = new SparseIntArray();
+    sparseIntArray.append(5, 3);


Can we also add an entry that is neither a slow/frozen frame to ensure that we exclude that as a part of calculation?

Sure. Done!

jeremyjiang-dev · 2022-03-25T13:33:22Z

The public api surface has changed for the subproject firebase-perf: error: Added class com.google.firebase.perf.metrics.FrameMetricsCalculator [AddedClass]
Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

Has the public surface changed at all? Can we make sure to have the visibility of APIs rightly set?

This is a stale comment as I have fixed the issue in a following commit :)

jeremyjiang-dev · 2022-03-25T15:45:08Z

/retest

jeremyjiang-dev · 2022-03-25T15:50:33Z

/retest

* Update CHANGELOG from m112 release * include full link to docsite * Fix typo

… in the given Map (#3518)

#3538 * Added catching RuntimeException when trying to get the analytics data Bundle from the Activity Intent since it's possible that the extras could contain invalid or incorrectly formatted data that will throw an Exception when trying to get the Bundle.

* move screen trace start to onResum * gjf * comment * fix tests

This reverts commit 1667f33.

* Fix refresh logic for App Check custom providers. * Update tests. * Extract 1000L into a constant.

* Extending test-app * Removing comment * responding to comments * Delete MainActivityTest.java * running linter * removing unnecessary additions to gradle * Fixing lint errors Co-authored-by: Manny Jimenez <[email protected]>

Co-authored-by: rachaprince <[email protected]>

google-oss-bot · 2022-03-25T20:21:21Z

@jeremyjiang-dev: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
copyright-check	`79b62dc`	link	`/test copyright-check`
api-information	`79b62dc`	link	`/test api-information`
check-changed	`79b62dc`	link	`/test check-changed`
smoke-tests	`79b62dc`	link	`/test smoke-tests`
device-check-changed	`79b62dc`	link	`/test device-check-changed`
check-coverage-changed	`79b62dc`	link	`/test check-coverage-changed`
binary-size	`79b62dc`	link	`/run binary-size`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

google-oss-bot added the size/L label Mar 23, 2022

jeremyjiang-dev changed the title ~~Capture frame metrics in fragment mintor~~ Capture frame metrics in fragment monitor Mar 23, 2022

jeremyjiang-dev changed the base branch from perfFragmentsEAP to fragment-trace March 23, 2022 20:43

Base automatically changed from fragment-trace to perfFragmentsEAP March 23, 2022 23:05

jeremyjiang-dev force-pushed the fragment-fma branch from 9bc559e to 8757b15 Compare March 24, 2022 16:10

jeremyjiang-dev requested review from leotianlizhan and visumickey and removed request for leotianlizhan March 24, 2022 16:42

leotianlizhan approved these changes Mar 24, 2022

View reviewed changes

firebase-perf/src/test/java/com/google/firebase/perf/application/FragmentStateMonitorTest.java Outdated Show resolved Hide resolved

visumickey reviewed Mar 25, 2022

View reviewed changes

google-oss-bot added size/XL and removed size/L labels Mar 25, 2022

jeremyjiang-dev changed the base branch from perfFragmentsEAP to master March 25, 2022 15:27

jeremyjiang-dev changed the base branch from master to perfFragmentsEAP March 25, 2022 15:27

jeremyjiang-dev changed the base branch from perfFragmentsEAP to master March 25, 2022 15:43

google-oss-bot added size/XXL and removed size/XL labels Mar 25, 2022

jeremyjiang-dev changed the base branch from master to perfFragmentsEAP March 25, 2022 15:43

google-oss-bot added size/XL and removed size/XXL labels Mar 25, 2022

jeremyjiang-dev added 5 commits March 25, 2022 15:43

Add tests for FrameMetricsCalculator

e9c8f4f

Add tests for FragmentStateMonitor

4740cfd

Add more comments.

5b129bb

Add copyright

4cdc767

Fix style.

a0e6491

jeremyjiang-dev force-pushed the fragment-fma branch from 2f24380 to a0e6491 Compare March 25, 2022 19:43

google-oss-bot added size/L and removed size/XXL labels Mar 25, 2022

rachaprince and others added 15 commits March 25, 2022 16:18

Update CHANGELOG from m112 release (#3547)

1d2f3fa

* Update CHANGELOG from m112 release * include full link to docsite * Fix typo

Change DocumentOverlayCache.saveOverlays() to require non-null values…

4981e4a

… in the given Map (#3518)

Don't use Indexes for outdated limit queries (#3549)

6e0944a

Add Firebase App Distribution to smoke tests (#3555)

f29304d

Use assertThrows() in StorageReferenceTest (#3572)

9d8a39c

Fireperf: start screen trace in onResume (#3532)

4b5e63b

* move screen trace start to onResum * gjf * comment * fix tests

Revert "Add Firebase App Distribution to smoke tests (#3555)" (#3576)

998f1d6

This reverts commit 1667f33.

Java doc updates as per b/190099349 (#3580)

1214ac2

Firestore: Migrate unit tests to assertThrows() (#3562)

a5c4862

Fix refresh logic for App Check custom providers. (#3582)

2999ae7

* Fix refresh logic for App Check custom providers. * Update tests. * Extract 1000L into a constant.

[firehorn] Revert "Workaround to make smoke-test pass. (#3531)" (#3543)

15dd6de

Support App Distribution in smoke tests (#3585)

3e05a7e

Co-authored-by: rachaprince <[email protected]>

Address comments.

79b62dc

google-oss-bot added size/XXL and removed size/L labels Mar 25, 2022

jeremyjiang-dev changed the title ~~Capture frame metrics in fragment monitor~~ [Obsolete] Capture frame metrics in fragment monitor Mar 25, 2022

jeremyjiang-dev closed this Mar 25, 2022

jeremyjiang-dev mentioned this pull request Mar 25, 2022

Add frame metrics to fragment traces #3592

Merged

firebase locked and limited conversation to collaborators Apr 25, 2022

[Obsolete] Capture frame metrics in fragment monitor #3584

[Obsolete] Capture frame metrics in fragment monitor #3584

Uh oh!

Conversation

jeremyjiang-dev commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

Test Logs

Notes

Uh oh!

google-oss-bot commented Mar 23, 2022

Uh oh!

google-oss-bot commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

base

firebase-abt

firebase-annotations

firebase-common

firebase-components

firebase-config

firebase-datatransport

firebase-encoders

firebase-encoders-json

firebase-encoders-proto

firebase-installations

firebase-installations-interop

firebase-perf

protolite-well-known-types

transport-api

transport-backend-cct

transport-runtime

Test Logs

Notes

Uh oh!

google-oss-bot commented Mar 23, 2022

Uh oh!

Uh oh!

jeremyjiang-dev commented Mar 24, 2022

Uh oh!

visumickey commented Mar 25, 2022

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!

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!

jeremyjiang-dev commented Mar 25, 2022

Uh oh!

jeremyjiang-dev commented Mar 23, 2022 •

edited

Loading

google-oss-bot commented Mar 23, 2022 •

edited

Loading

Coverage Report ¹

google-oss-bot commented Mar 23, 2022 •

edited

Loading

Size Report ¹

`base`

`firebase-abt`

`firebase-annotations`

`firebase-common`

`firebase-components`

`firebase-config`

`firebase-datatransport`

`firebase-encoders`

`firebase-encoders-json`

`firebase-encoders-proto`

`firebase-installations`

`firebase-installations-interop`

`firebase-perf`

`protolite-well-known-types`

`transport-api`

`transport-backend-cct`

`transport-runtime`