Skip to content

Commit 7279a02

Browse files
jeremyjiang-devjeremyjiang-dev
committed
Address review comments.
1 parent 8fd8d59 commit 7279a02

File tree

5 files changed

+114
-38
lines changed

5 files changed

+114
-38
lines changed

‎firebase-perf/src/main/java/com/google/firebase/perf/application/AppStateMonitor.java

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@
3434
import com.google.firebase.perf.util.Clock;
3535
import com.google.firebase.perf.util.Constants;
3636
import com.google.firebase.perf.util.Constants.CounterNames;
37+
import com.google.firebase.perf.util.ScreenTraceUtil;
3738
import com.google.firebase.perf.util.Timer;
38-
import com.google.firebase.perf.util.Utils;
3939
import com.google.firebase.perf.v1.ApplicationProcessState;
4040
import com.google.firebase.perf.v1.TraceMetric;
4141
import java.lang.ref.WeakReference;
@@ -362,30 +362,7 @@ private void sendScreenTrace(Activity activity) {
362362
// All metrics are zero, no need to send screen trace.
363363
return;
364364
}
365-
// Only putMetric if corresponding metric is non-zero.
366-
if (frameMetrics.getTotalFrames() > 0) {
367-
screenTrace.putMetric(
368-
Constants.CounterNames.FRAMES_TOTAL.toString(), frameMetrics.getTotalFrames());
369-
}
370-
if (frameMetrics.getSlowFrames() > 0) {
371-
screenTrace.putMetric(
372-
Constants.CounterNames.FRAMES_SLOW.toString(), frameMetrics.getSlowFrames());
373-
}
374-
if (frameMetrics.getFrozenFrames() > 0) {
375-
screenTrace.putMetric(
376-
Constants.CounterNames.FRAMES_FROZEN.toString(), frameMetrics.getFrozenFrames());
377-
}
378-
if (Utils.isDebugLoggingEnabled(activity.getApplicationContext())) {
379-
logger.debug(
380-
"sendScreenTrace name:"
381-
+ getScreenTraceName(activity)
382-
+ " _fr_tot:"
383-
+ frameMetrics.getTotalFrames()
384-
+ " _fr_slo:"
385-
+ frameMetrics.getSlowFrames()
386-
+ " _fr_fzn:"
387-
+ frameMetrics.getFrozenFrames());
388-
}
365+
ScreenTraceUtil.addFrameCounters(screenTrace, frameMetrics);
389366
// Stop and record trace
390367
screenTrace.stop();
391368
}

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

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.firebase.perf.transport.TransportManager;
2727
import com.google.firebase.perf.util.Clock;
2828
import com.google.firebase.perf.util.Constants;
29+
import com.google.firebase.perf.util.ScreenTraceUtil;
2930
import java.util.WeakHashMap;
3031

3132
public class FragmentStateMonitor extends FragmentManager.FragmentLifecycleCallbacks {
@@ -109,17 +110,8 @@ public void onFragmentPaused(@NonNull FragmentManager fm, @NonNull Fragment f) {
109110
// All metrics are zero, no need to send screen trace.
110111
return;
111112
}
112-
// Only putMetric if corresponding metric is non-zero.
113-
if (totalFrames > 0) {
114-
fragmentTrace.putMetric(Constants.CounterNames.FRAMES_TOTAL.toString(), totalFrames);
115-
}
116-
if (slowFrames > 0) {
117-
fragmentTrace.putMetric(Constants.CounterNames.FRAMES_SLOW.toString(), slowFrames);
118-
}
119-
if (frozenFrames > 0) {
120-
fragmentTrace.putMetric(Constants.CounterNames.FRAMES_FROZEN.toString(), frozenFrames);
121-
}
122-
113+
ScreenTraceUtil.addFrameCounters(
114+
fragmentTrace, new FrameMetrics(totalFrames, slowFrames, frozenFrames));
123115
fragmentTrace.stop();
124116
}
125117

‎firebase-perf/src/main/java/com/google/firebase/perf/util/ScreenTraceUtil.java

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// Copyright 2022 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
//
6+
// You may obtain a copy of the License at
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.firebase.perf.util;
16+
17+
import com.google.firebase.perf.logging.AndroidLogger;
18+
import com.google.firebase.perf.metrics.FrameMetricsCalculator.FrameMetrics;
19+
import com.google.firebase.perf.metrics.Trace;
20+
21+
/** Utility class for screen traces. */
22+
public class ScreenTraceUtil {
23+
private static final AndroidLogger logger = AndroidLogger.getInstance();
24+
25+
/**
26+
* Set the metrics of total frames, slow frames, and frozen frames for the given screen trace.
27+
*
28+
* @param screenTrace a screen trace
29+
* @param frameMetrics frame metrics calculated by {@link
30+
* com.google.firebase.perf.metrics.FrameMetricsCalculator#calculateFrameMetrics}
31+
* @return the screen trace with frame metrics added.
32+
*/
33+
public static Trace addFrameCounters(Trace screenTrace, FrameMetrics frameMetrics) {
34+
// Only putMetric if corresponding metric is greater than zero.
35+
if (frameMetrics.getTotalFrames() > 0) {
36+
screenTrace.putMetric(
37+
Constants.CounterNames.FRAMES_TOTAL.toString(), frameMetrics.getTotalFrames());
38+
}
39+
if (frameMetrics.getSlowFrames() > 0) {
40+
screenTrace.putMetric(
41+
Constants.CounterNames.FRAMES_SLOW.toString(), frameMetrics.getSlowFrames());
42+
}
43+
if (frameMetrics.getFrozenFrames() > 0) {
44+
screenTrace.putMetric(
45+
Constants.CounterNames.FRAMES_FROZEN.toString(), frameMetrics.getFrozenFrames());
46+
}
47+
logger.debug(
48+
"Screen trace: "
49+
+ screenTrace.getClass().getSimpleName()
50+
+ " _fr_tot:"
51+
+ frameMetrics.getTotalFrames()
52+
+ " _fr_slo:"
53+
+ frameMetrics.getSlowFrames()
54+
+ " _fr_fzn:"
55+
+ frameMetrics.getFrozenFrames());
56+
return screenTrace;
57+
}
58+
}

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

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,16 +134,34 @@ public void lifecycleCallbacks_differentFrameMetricsCapturedByFma_logFragmentScr
134134
}
135135

136136
@Test
137-
public void lifecycleCallbacks_sameFrameMetricsCapturedByFma_dropFragmentScreenTrace() {
137+
public void lifecycleCallbacks_onPausedCalledTwice_logFragmentScreenTraceOnce() {
138138
FragmentStateMonitor monitor =
139139
new FragmentStateMonitor(clock, mockTransportManager, appStateMonitor, fma);
140140
when(fma.getMetrics()).thenReturn(fmaMetrics1);
141141
monitor.onFragmentResumed(mockFragmentManager, mockFragment);
142142
verify(mockTransportManager, times(0)).log(any(TraceMetric.class), any());
143143

144+
when(fma.getMetrics()).thenReturn(fmaMetrics2);
145+
monitor.onFragmentPaused(mockFragmentManager, mockFragment);
146+
verify(mockTransportManager, times(1)).log(any(TraceMetric.class), any());
147+
148+
when(fma.getMetrics()).thenReturn(fmaMetrics2);
149+
monitor.onFragmentPaused(mockFragmentManager, mockFragment);
150+
verify(mockTransportManager, times(1)).log(any(TraceMetric.class), any());
151+
}
152+
153+
@Test
154+
public void lifecycleCallbacks_onPausedCalledBeforeOnResume_doesNotLogFragmentScreenTrace() {
155+
FragmentStateMonitor monitor =
156+
new FragmentStateMonitor(clock, mockTransportManager, appStateMonitor, fma);
144157
when(fma.getMetrics()).thenReturn(fmaMetrics1);
158+
145159
monitor.onFragmentPaused(mockFragmentManager, mockFragment);
146160
verify(mockTransportManager, times(0)).log(any(TraceMetric.class), any());
161+
162+
when(fma.getMetrics()).thenReturn(fmaMetrics2);
163+
monitor.onFragmentResumed(mockFragmentManager, mockFragment);
164+
verify(mockTransportManager, times(0)).log(any(TraceMetric.class), any());
147165
}
148166

149167
@Test
@@ -207,7 +225,20 @@ public void lifecycleCallbacks_cleansUpMap_duringActivityTransitions() {
207225
}
208226

209227
@Test
210-
public void fragmentTraceCreation_dropsTrace_whenFragmentNameTooLong() {
228+
public void fragmentTraceCreation_whenFrameMetricsIsUnchanged_dropsTrace() {
229+
FragmentStateMonitor monitor =
230+
new FragmentStateMonitor(clock, mockTransportManager, appStateMonitor, fma);
231+
when(fma.getMetrics()).thenReturn(fmaMetrics1);
232+
monitor.onFragmentResumed(mockFragmentManager, mockFragment);
233+
verify(mockTransportManager, times(0)).log(any(TraceMetric.class), any());
234+
235+
when(fma.getMetrics()).thenReturn(fmaMetrics1);
236+
monitor.onFragmentPaused(mockFragmentManager, mockFragment);
237+
verify(mockTransportManager, times(0)).log(any(TraceMetric.class), any());
238+
}
239+
240+
@Test
241+
public void fragmentTraceCreation_whenFragmentNameTooLong_dropsTrace() {
211242
AppStateMonitor appStateMonitor =
212243
spy(new AppStateMonitor(mockTransportManager, clock, configResolver, fma));
213244
FragmentStateMonitor fragmentMonitor =

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,22 @@ public void calculateFrameMetrics_validSparseIntArray_returnsCorrectFrameMetrics
9191
assertThat(metrics.getSlowFrames()).isEqualTo(2);
9292
assertThat(metrics.getFrozenFrames()).isEqualTo(2);
9393
}
94+
95+
@Test
96+
public void
97+
calculateFrameMetrics_validSparseIntArrayWithoutSlowFramesOrFrozenFrames_returnsCorrectFrameMetrics() {
98+
// Slow frames have duration greater than 16ms and frozen frames have duration greater than
99+
// 700ms. The key value pair means (duration, num_of_samples).
100+
SparseIntArray sparseIntArray = new SparseIntArray();
101+
sparseIntArray.append(5, 3);
102+
SparseIntArray[] arr = new SparseIntArray[1];
103+
arr[FrameMetricsAggregator.TOTAL_INDEX] = sparseIntArray;
104+
105+
FrameMetricsCalculator.FrameMetrics metrics = FrameMetricsCalculator.calculateFrameMetrics(arr);
106+
107+
// we should expect 3 total frames, 0 slow frames, and 0 frozen frames.
108+
assertThat(metrics.getTotalFrames()).isEqualTo(3);
109+
assertThat(metrics.getSlowFrames()).isEqualTo(0);
110+
assertThat(metrics.getFrozenFrames()).isEqualTo(0);
111+
}
94112
}

0 commit comments

Comments
 (0)