Fireperf: encapsulate FrameMetricsAggregator in a new object called FrameMetricsRecorder

[leotianlizhan]

b/229123892

TLDR

Add a new class called FrameMetricsRecorder that is essentially an encapsulation of FrameMetricsAggregator. Every activity will have an instance of FrameMetricsRecorder.

Timeline Graph to Understand The Relationship between Activity, Fragments, and FrameMetricsAggregator

This an example of what is happening on a timeline. Fragment can be ANY UI object that is meaningful. e.g.) a listview that caused performance problems before, want to have screen traces for it to catch when such happens again in the future.

Background

Following the stopping screen trace in onActivityPostPaused change and realizing that is unviable due to onActivityPostPause being only available in API 29+, a new design is needed for screen rendering tracing.

Why encapsulate FMA

There's 2 main reasons:

1. Relieve some responsibility from AppStateMonitor, a class with too much responsibility
2. Structurally prevent FMA misuse in the future by anyone working on Fireperf Android SDK

Classes like AppStateMonitor are very heavy with many responsibilities: manage FrameMetricsAggregator, instrument, create, and build proto for screen trace and _fs/_bs traces, etc. All which are on top of its responsibility of registering itself for activity lifecycle callbacks, which is already a lot of boilerplate to read through. This makes it hard to test and debug.

Massive “god classes” like AppStateMonitor are also error prone and any small change could break it. FrameMetricsAggregator is an example of an intricate class that requires special knowledge to handle, and that is only one responsibility in a very bloated class AppStateMonitor. To debug the Activity screen trace bug, you need to read through other code that is unrelated to screen traces, which is adding a lot more mental load to figure out how everything interacts with each other.

Responsibility of FrameMetricsRecorder

A manager for FrameMetricsAggregator, includes FMA instantiation, start activity recording, stop activity recording, calculating differences in FrameMetricsAggregator’s data for a sub-trace (e.g. Fragment trace). Basically it only has a single responsibility: abstract away FrameMetricsAggregator and anything related to it.

Design Rationale

Structurally enforces only 1 activity is being passed into each FrameMetricsAggregator.

• Each instance takes a single Activity in the constructor, and creates a single private FrameMetricsAggregator.
• Prevents Inter-Activity Behavior and Screen Trace.

Structurally prevent misuse of FrameMetricsAggregator

• By abstracting away all that relates to FrameMetricsAggregator, guards can be put in place to prevent misuse.
  e.g.) in start(), can check if FMA is already recording, if it is, don’t allow it.
• Detecting misuse then throwing errors helps surfacing Inter-Activity Behavior and Screen Trace, otherwise this bug was difficult to find.

Simplify code and encapsulate responsibility

• Inputs and outputs use PerfFrameMetrics, FMA is completely contained and encapsulated within this class.
• AppStateMonitor doesn't manage FrameMetricsAggregator, it simply calls start() and stop().
• Each method has only a single purpose, easy to test unlike many big multi-purpose methods in other classes.

// AppStateMonitor.java
private void onActivityCreated(Activity a) {
  FrameMetricsRecorder recorder = new FrameMetricsRecorder(a);
  recorderMap.put(a, recorder);
  …
}
private void onActivityStarted(Activity a) {
  recorderMap.get(a).start();
  …
  // create a duration trace…
  …  
}
private void onActivityStopped(Activity a) {
  PerfFrameMetrics data = recorderMap.get(a).stop()
  …
  // add data to duration trace then send it
  …  
}

Structure

[google-oss-bot]

Coverage Report ¹

Affected Products

• firebase-perf

  Overall coverage changed from 71.36% (51cc21f) to 71.44% (1c86195) by +0.08%.

  Filename	Base (51cc21f)	Merge (1c86195)	Diff
  FrameMetricsCalculator.java	96.30%	96.77%	+0.48%
  FrameMetricsRecorder.java	?	77.14%	?

Test Logs

• Base (51cc21f): https://android-ci.firebaseopensource.com/view/gcs/android-ci/logs/postsubmit-check-coverage/1517315851424894977
• Merge (1c86195): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3665/check-coverage-changed/1519376825778704389

Notes

• Commit (1c86195) is created by Prow via merging PR base commit (51cc21f) and head commit (27300b9).
• 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/.

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

[google-oss-bot]

Size Report ¹

Affected Products

• firebase-perf

  Type	Base (51cc21f)	Merge (1c86195)	Diff
  aar	308 kB	311 kB	+2.80 kB (+0.9%)
  apk (release)	2.47 MB	2.47 MB	+892 B (+0.0%)

Test Logs

• Base (51cc21f): https://android-ci.firebaseopensource.com/view/gcs/android-ci/logs/postsubmit-binary-size/1517315851424894976
• Merge (1c86195): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3665/binary-size/1519376825782898690

Notes

• Commit (1c86195) is created by Prow via merging PR base commit (51cc21f) and head commit (27300b9).

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

[jposuna]

Before reviewing the changes themselves, can you explain why we want to have this?

[jeremyjiang-dev]

Could you mark this PR to ready for review and I can take another look?
Would you like to merge this PR to a feature branch instead of master for all the frame metrics refactor work?

[leotianlizhan]

Could you mark this PR to ready for review and I can take another look? Would you like to merge this PR to a feature branch instead of master for all the frame metrics refactor work?

Done. I changed base branch to a new feature branch

[leotianlizhan]

/retest

[leotianlizhan]

/retest

[visumickey]

Can you also expose an API isRecordingFrameMetricsForActivity(Activity activity)? This would ensure that the caller knows the current state before starting to track an activity.

[leotianlizhan]

I am not sure what value that API would add because I don't see us using it. We are just going to call start() in onActivityStart and stop() in onActivityStop, if it fails then we are not doing symmetrical calls and we are calling start() somewhere else, which is wrong and it should output warning.

In addition I don't think we should expose internal state, that's opening the blackbox. if start() stop() are called wrong and causes internal state to be problematic, that's a problem that should be contained in the blackbox, all the outside has to know is that "I used this black box in the wrong way".

[jposuna]

+1 to Leo

[visumickey]

I'm mixed on this. I have seen usecases where having such an API would be very useful. But in the interest of self-containment, I'm fine to move ahead here.

[jposuna]

Thanks for working on this and spending the time to make the changes!

[jeremyjiang-dev]

Thanks Leo for your hard work! Great work!