Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev: measure app start TTI more accurately #13997

Merged
merged 10 commits into from
Jan 30, 2023
1 change: 1 addition & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ const CONST = {
SWITCH_REPORT: 'switch_report',
SIDEBAR_LOADED: 'sidebar_loaded',
COLD: 'cold',
WARM: 'warm',
REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME: 1500,
SHOW_LOADING_SPINNER_DEBOUNCE_TIME: 250,
TOOLTIP_SENSE: 1000,
Expand Down
18 changes: 18 additions & 0 deletions src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ import React, {Component} from 'react';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';

import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator';
import ONYXKEYS from '../../../ONYXKEYS';
import SCREENS from '../../../SCREENS';
import Permissions from '../../Permissions';
import Timing from '../../actions/Timing';
import CONST from '../../../CONST';

// Screens
import ReportScreen from '../../../pages/home/ReportScreen';
Expand Down Expand Up @@ -57,7 +60,12 @@ const getInitialReportScreenParams = (reports, ignoreDefaultRooms, policies) =>
class MainDrawerNavigator extends Component {
constructor(props) {
super(props);
this.trackAppStartTiming = this.trackAppStartTiming.bind(this);
this.initialParams = getInitialReportScreenParams(props.reports, !Permissions.canUseDefaultRooms(props.betas), props.policies);

// When we have chat reports the moment this component got created
// we know that the data was served from storage/cache
this.isFromCache = _.size(props.reports) > 0;
}

shouldComponentUpdate(nextProps) {
Expand All @@ -70,6 +78,15 @@ class MainDrawerNavigator extends Component {
return true;
}

trackAppStartTiming() {
// We only want to report timing events when rendering from cached data
if (!this.isFromCache) {
return;
}

Timing.end(CONST.TIMING.SIDEBAR_LOADED);
}

render() {
// Wait until reports are fetched and there is a reportID in initialParams
if (!this.initialParams.reportID) {
Expand All @@ -87,6 +104,7 @@ class MainDrawerNavigator extends Component {
return (
<SidebarScreen
navigation={navigation}
onLayout={this.trackAppStartTiming}
reportIDFromRoute={reportIDFromRoute}
/>
);
Expand Down
2 changes: 0 additions & 2 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';
import Log from '../Log';
import Performance from '../Performance';
import Timing from './Timing';
import * as Policy from './Policy';
import Navigation from '../Navigation/Navigation';
import ROUTES from '../../ROUTES';
Expand Down Expand Up @@ -95,7 +94,6 @@ function setSidebarLoaded() {
}

Onyx.set(ONYXKEYS.IS_SIDEBAR_LOADED, true);
Timing.end(CONST.TIMING.SIDEBAR_LOADED);
Performance.markEnd(CONST.TIMING.SIDEBAR_LOADED);
Performance.markStart(CONST.TIMING.REPORT_INITIAL_RENDER);
}
Expand Down
39 changes: 21 additions & 18 deletions src/libs/actions/Timing.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,29 +32,32 @@ function end(eventName, secondaryName = '') {
return;
}

const {startTime, shouldUseFirebase} = timestampData[eventName];
const eventTime = Date.now() - startTime;
Environment.getEnvironment().then((envName) => {
const {startTime, shouldUseFirebase} = timestampData[eventName];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this inside a promise caused #33290

const eventTime = Date.now() - startTime;

if (shouldUseFirebase) {
Firebase.stopTrace(eventName);
}
if (shouldUseFirebase) {
Firebase.stopTrace(eventName);
}

const grafanaEventName = secondaryName
? `expensify.cash.${eventName}.${secondaryName}`
: `expensify.cash.${eventName}`;
const baseEventName = `${envName}.new.expensify.${eventName}`;
const grafanaEventName = secondaryName
? `${baseEventName}.${secondaryName}`
: baseEventName;

console.debug(`Timing:${grafanaEventName}`, eventTime);
delete timestampData[eventName];
console.debug(`Timing:${grafanaEventName}`, eventTime);
delete timestampData[eventName];

if (Environment.isDevelopment()) {
// Don't create traces on dev as this will mess up the accuracy of data in release builds of the app
return;
}
if (Environment.isDevelopment()) {
// Don't create traces on dev as this will mess up the accuracy of data in release builds of the app
return;
}

API.write('SendPerformanceTiming', {
name: grafanaEventName,
value: eventTime,
platform: `${getPlatform()}`,
API.write('SendPerformanceTiming', {
name: grafanaEventName,
value: eventTime,
platform: `${getPlatform()}`,
});
});
}

Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class ReportActionsView extends React.Component {
this.didLayout = false;
this.didSubscribeToReportTypingEvents = false;
this.unsubscribeVisibilityListener = null;
this.hasCachedActions = _.size(props.reportActions) > 0;

// We need this.sortedAndFilteredReportActions to be set before this.state is initialized because the function to calculate the newMarkerReportActionID uses the sorted report actions
this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(props.reportActions);
Expand Down Expand Up @@ -348,7 +349,7 @@ class ReportActionsView extends React.Component {
}

this.didLayout = true;
Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD);
Timing.end(CONST.TIMING.SWITCH_REPORT, this.hasCachedActions ? CONST.Timing.WARM : CONST.TIMING.COLD);
Copy link
Contributor

@aimane-chnaif aimane-chnaif Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed typo here.
@hannojg can you please fix that and raise PR quickly?
NVM, I raised PR to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aimane-chnaif Thanks for this comment, I was coming here for the same as I have reported here too.


// Capture the init measurement only once not per each chat switch as the value gets overwritten
if (!ReportActionsView.initMeasured) {
Expand Down
5 changes: 5 additions & 0 deletions src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ const propTypes = {
/** Current reportID from the route in react navigation state object */
reportIDFromRoute: PropTypes.string,

/** Callback when onLayout of sidebar is called */
onLayout: PropTypes.func,

/** Whether we are viewing below the responsive breakpoint */
isSmallScreenWidth: PropTypes.bool.isRequired,

Expand All @@ -78,6 +81,7 @@ const defaultProps = {
avatar: ReportUtils.getDefaultAvatar(),
},
reportIDFromRoute: '',
onLayout: () => {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I see you added here.
I checked code from main instead of your branch.

priorityMode: CONST.PRIORITY_MODE.DEFAULT,
};

Expand Down Expand Up @@ -189,6 +193,7 @@ class SidebarLinks extends React.Component {
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
onLayout={() => {
this.props.onLayout();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail unit testing. Let's add onLayout prop here too:

<SidebarLinks
onLinkClick={() => {}}
insets={{
top: 0,
left: 0,
right: 0,
bottom: 0,
}}
isSmallScreenWidth={false}
reportIDFromRoute={reportIDFromRoute}
/>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why it will fail unit testing? I set for onLayout a default prop, so that shouldn't be an issue?
(Also when running the tests manually they all succeed 👀 )

App.setSidebarLoaded();
this.isSidebarLoaded = true;
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class BaseSidebarScreen extends Component {
isSmallScreenWidth={this.props.isSmallScreenWidth}
isDrawerOpen={this.props.isDrawerOpen}
reportIDFromRoute={this.props.reportIDFromRoute}
onLayout={this.props.onLayout}
/>
</View>
<KeyboardShortcutsModal />
Expand Down
3 changes: 3 additions & 0 deletions src/pages/home/sidebar/SidebarScreen/sidebarPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@ const sidebarPropTypes = {

/** reportID in the current navigation state */
reportIDFromRoute: PropTypes.string,

/** Callback when onLayout of sidebar is called */
onLayout: PropTypes.func,
};
export default sidebarPropTypes;
Loading