Add performance metrics to the app #7953

larkox · 2024-05-14T16:27:37Z

Release Note

Mobile will now send performance metrics to servers that have them enabled

enahum · 2024-05-14T18:27:47Z

app/components/post_draft/post_draft.tsx

@@ -73,6 +74,11 @@ function PostDraft({
        setCursorPosition(message.length);
    }, [channelId, rootId]);

+    useEffect(() => {


What is the impact if you use useLayoutEffect instead?

useLayoutEffect will run before painting into the screen. I would prefer if the measure is done after painting. But 0/5.

ok I understand.. now the next question is why do we consider the channel/thread switch complete when the post_draft renders ? out of curiosity my question, as I want to understand how did you come up with the fact that this is the last component being rendered on screen.

Also, if that is the case, is it still true on tablets ?

Now that you mention it, I wanted to revisit this, you are right.
No, the post draft is not the last thing that renders in a channel. I thought it might be (and was convenient) but after some tests, it may not be.
Also, I completely forgot about the tablet scenario 🤦
I will revisit this.

app/managers/performance_metrics_manager.ts

app/screens/home/channel_list/channel_list.tsx

types/api/config.d.ts

app/screens/home/channel_list/categories_list/categories/categories.test.tsx

app/managers/performance_metrics_manager/types.d.ts

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

hmhealey · 2024-05-23T20:22:10Z

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

+            version: '0.1.0',
+            labels: {
+                platform: Platform.select({ios: 'ios', default: 'android'}),
+                agent: 'rnapp',


Similar to my comment on the server PR, I don't think we need to set the agent for mobile reports

not for this PR, but something we should consider is to come up with a way to identify the type of device (High end, Mid, low end) as that will give us more insight with the collected numbers.

That seems like a good idea. I've wondered about similar on the web-side, but we're much more restricted in what metrics we can collect about the device there

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

hmhealey · 2024-05-23T20:39:27Z

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

+        };
+    }
+
+    private onAppStateChange(appState: AppStateStatus) {


Should the PerformanceMetricsManager perhaps be the one to listen for the app state to change because there'll be multiple batchers for multiple

Alternatively, maybe we would want to send out any pending metrics as soon as you switch away from a server to avoid sending out too many requests to different servers when the app goes to the background

I didn't want to add more methods to the batcher interface (in this case, would be a flush method). But we could add it, yes.

We could also do as you say, and send the metrics on server switch, but that would have some impact (probably irrelevant though) to the server switch logic. I feel it is better to leave that performance impact to the "send app to the background" interaction.

I would agree with both of you.

leave the logic when the app is sent to the background.

move the event listener to the manager and call the sendBatch on each batcher from there.

Important to mention that any timer is paused when the app state changes to background

app/managers/performance_metrics_manager/index.ts

hmhealey · 2024-05-23T20:50:56Z

app/managers/performance_metrics_manager/index.ts

+        this.ensureBatcher(serverUrl).addToBatch({
+            metric: metricName,
+            value: (performanceNow - startTime) / 1000,
+            timestamp: Math.round(startTime + performanceNowSkew),


Is performance.timeOrigin not available for use here? I remember you mentioned that the performance API is only partially implemented on RN, but I don't know if that one is included

Nope. No performance origin apparently. Only performance.now.

Have you checked https://github.com/oblador/react-native-performance/blob/master/packages/react-native-performance/README.md ?

I thought I did, and dismissed it because it wasn't maintained for a long time, but I see the last release was March, so I may have mixed different libraries in my mind.

I will re-assess whether this library makes sense for what we want to do.

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

enahum · 2024-05-28T00:15:10Z

app/components/post_draft/post_draft.tsx

@@ -73,6 +74,11 @@ function PostDraft({
        setCursorPosition(message.length);
    }, [channelId, rootId]);

+    useEffect(() => {


ok I understand.. now the next question is why do we consider the channel/thread switch complete when the post_draft renders ? out of curiosity my question, as I want to understand how did you come up with the fact that this is the last component being rendered on screen.

Also, if that is the case, is it still true on tablets ?

enahum · 2024-05-28T00:18:46Z

app/components/team_sidebar/team_list/team_item/team_item.tsx

+        if (!team) {
+            return;
+        }
+        if (selected) {
+            return;
+        }


this could be simplified as

Suggested change

if (!team) {

return;

}

if (selected) {

return;

}

if (!team || selected) {

return;

}

enahum · 2024-05-28T00:42:09Z

app/managers/performance_metrics_manager/index.ts

+        this.ensureBatcher(serverUrl).addToBatch({
+            metric: metricName,
+            value: (performanceNow - startTime) / 1000,
+            timestamp: Math.round(startTime + performanceNowSkew),


Have you checked https://github.com/oblador/react-native-performance/blob/master/packages/react-native-performance/README.md ?

enahum · 2024-05-28T00:45:26Z

app/managers/performance_metrics_manager/index.ts

+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {getTimeSinceStartup} from 'react-native-startup-time';


I think you can use https://github.com/oblador/react-native-performance/blob/master/packages/react-native-performance instead of startup-time, it has all you need for perf metrics as well as start end time but also you can have values for when the JS bundle starts and ends loading and a bunch of other metrics for RN.

more importantly it does already support the new RN arch for when we migrate.

enahum · 2024-05-28T00:48:47Z

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

+            version: '0.1.0',
+            labels: {
+                platform: Platform.select({ios: 'ios', default: 'android'}),
+                agent: 'rnapp',


not for this PR, but something we should consider is to come up with a way to identify the type of device (High end, Mid, low end) as that will give us more insight with the collected numbers.

enahum · 2024-05-28T00:50:45Z

app/managers/performance_metrics_manager/performance_metrics_batcher.ts

+        };
+    }
+
+    private onAppStateChange(appState: AppStateStatus) {


I would agree with both of you.

leave the logic when the app is sent to the background.

move the event listener to the manager and call the sendBatch on each batcher from there.

Important to mention that any timer is paused when the app state changes to background

enahum · 2024-05-28T00:56:20Z

package.json

@@ -87,6 +87,7 @@
    "react-native-section-list-get-item-layout": "2.2.3",
    "react-native-shadow-2": "7.0.8",
    "react-native-share": "10.1.0",
+    "react-native-startup-time": "2.1.0",


consider switching the library as suggested

types/api/config.d.ts

enahum · 2024-05-30T14:49:11Z

Hey @larkox you sure this is ok? There seems to be a lot of unrelated changes

larkox · 2024-05-30T14:55:27Z

@enahum A bad merge for some reason... will fix it.

larkox · 2024-05-30T15:48:34Z

@enahum Bad merge fixed

hmhealey · 2024-05-31T17:12:18Z

app/components/team_sidebar/team_list/team_item/team_item.tsx

+
+        PerformanceMetricsManager.startMetric('mobile_team_switch');
+        handleTeamChange(serverUrl, team.id);
+    }, [selected]);


Doesn't this callback also need to depend on the server URL and team ID?

Yes ot depends on the team and the team.id but the serverUrl can be omitted as I believe the component is remounted when switching servers

hmhealey · 2024-05-31T17:13:30Z

app/managers/performance_metrics_manager/index.ts

+    }
+
+    private onAppStateChange(appState: AppStateStatus) {
+        const isAppStateActive = appState === 'active';


Similar to my comments on your PR about the websocket, I don't think we actually need to store the previous value of this if this is only called when the app state changes

There are several "non active states", and this is executed when the state become non active. Calling several times forceSend shouldn't have any effect, but still, there is no need to do so. So I think there is not a big issue to store the previous value, to only call this the first time we go to an "inactive state".

hmhealey · 2024-05-31T17:14:30Z

app/managers/performance_metrics_manager/index.ts

+        this.ensureBatcher(serverUrl).addToBatch({
+            metric: 'mobile_load',
+            value: measure.duration,
+            timestamp: Date.now(),


Now that we have that library, could thse use performance.timeOrigin + measure.startTime?

I got many issues to make this sum up. This is the values I get from the library (on a real android device):

{"dateNow": 1718095572718, "dur": 401.2168229999952, "origin": 553634.5101, "startTime": 623363.507261, "sum": 1176998.017361, "timestamp": 1718095642446.9973}

The timestamp one is calculated as stated here: https://github.com/oblador/react-native-performance/blob/master/packages/react-native-performance/README.md

So... the only value that makes sense to use here is Date.now. The timestamp is also not so important, since it is only used for bucketing, and we are not going so fine grain down to seconds.

If you have any other idea, I am happy to try 😄

Huh, I feel like that doesn't match how the browser API works, but if that's how they say to do it, that's fine by me

app/managers/performance_metrics_manager/index.ts

enahum · 2024-06-07T00:57:14Z

app/components/post_list/post/post.tsx

@@ -60,6 +61,7 @@ type PostProps = {
    post: PostModel;
    rootId?: string;
    previousPost?: PostModel;
+    nextPost?: PostModel;


Perhaps you can just pass the boolean isLastPost instead of the entire model?

enahum · 2024-06-07T00:59:12Z

app/components/team_sidebar/team_list/team_item/team_item.tsx

+
+        PerformanceMetricsManager.startMetric('mobile_team_switch');
+        handleTeamChange(serverUrl, team.id);
+    }, [selected]);


Yes ot depends on the team and the team.id but the serverUrl can be omitted as I believe the component is remounted when switching servers

enahum · 2024-06-07T01:00:26Z

app/managers/performance_metrics_manager/index.ts

+        this.ensureBatcher(serverUrl).addToBatch({
+            metric: 'mobile_load',
+            value: measure.duration,
+            timestamp: Date.now(),


enahum · 2024-06-07T01:01:29Z

package.json

@@ -199,6 +200,9 @@
    "updatesnapshot": "jest --updateSnapshot"
  },
  "overrides": {
+    "react-native-startup-time": {


No need for this

test/test_helper.ts

larkox · 2024-06-11T17:52:35Z

@yasserfaraazkhan For now, the test for this is just a quick smoke test that nothing is broken. Specially on load, channel switch and team switch.

yasserfaraazkhan · 2024-06-11T20:17:17Z

not able to build the app locally on windows machine. Will test it in release build.

mattermost-build · 2024-06-12T07:45:29Z

Cherry pick is scheduled.

* Add performance metrics to the app * add batcher and improve handling * Add tests * Fix test * Address feedback * Address feedback * Address feedback * update podfile (cherry picked from commit 5f01f9e)

* Add performance metrics to the app * add batcher and improve handling * Add tests * Fix test * Address feedback * Address feedback * Address feedback * update podfile (cherry picked from commit 5f01f9e) Co-authored-by: Daniel Espino García <larkox@gmail.com>

Add performance metrics to the app

b2b9d4e

larkox added 2: Dev Review Requires review by a core commiter Do Not Merge Should not be merged until this label is removed labels May 14, 2024

larkox requested a review from hmhealey May 14, 2024 16:27

mm-cloud-bot added the release-note label May 14, 2024

enahum reviewed May 14, 2024

View reviewed changes

hmhealey reviewed May 15, 2024

View reviewed changes

app/managers/performance_metrics_manager.ts Outdated Show resolved Hide resolved

app/managers/performance_metrics_manager.ts Outdated Show resolved Hide resolved

app/managers/performance_metrics_manager.ts Outdated Show resolved Hide resolved

larkox added 3 commits May 17, 2024 16:50

add batcher and improve handling

0cfc907

Add tests

b3c5c18

Fix test

4b93df5

larkox marked this pull request as ready for review May 22, 2024 10:56

hmhealey requested changes May 23, 2024

View reviewed changes

Address feedback

54f9bb8

larkox requested review from hmhealey and enahum May 27, 2024 09:45

enahum requested changes May 28, 2024

View reviewed changes

larkox requested a review from enahum May 30, 2024 14:41

Address feedback

a2f771f

larkox force-pushed the registerPerformanceMetrics branch from f1bb84c to a2f771f Compare May 30, 2024 15:47

Merge branch 'main' into registerPerformanceMetrics

0c81f6f

hmhealey reviewed May 31, 2024

View reviewed changes

marianunez added this to the v2.18.0 milestone Jun 6, 2024

amyblais added this to the v2.18.0 milestone Jun 6, 2024

enahum reviewed Jun 7, 2024

View reviewed changes

amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone Docs/Needed Requires documentation labels Jun 10, 2024

Merge branch 'main' into registerPerformanceMetrics

4282c00

larkox added 2 commits June 11, 2024 10:58

Address feedback

2dcf85e

update podfile

2f9b7fe

larkox requested review from hmhealey and enahum June 11, 2024 09:46

larkox removed the Do Not Merge Should not be merged until this label is removed label Jun 11, 2024

hmhealey approved these changes Jun 11, 2024

View reviewed changes

enahum approved these changes Jun 11, 2024

View reviewed changes

larkox added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core commiter labels Jun 11, 2024

larkox requested a review from yasserfaraazkhan June 11, 2024 17:51

yasserfaraazkhan added QA Deferred Agreement with QA that these changes will be tested post-merge Build App for Android Build the mobile app for Android and removed 3: QA Review Requires review by a QA tester labels Jun 11, 2024

larkox added 4: Reviews Complete All reviewers have approved the pull request and removed Build App for Android Build the mobile app for Android labels Jun 12, 2024

larkox merged commit 5f01f9e into mattermost:main Jun 12, 2024
31 of 32 checks passed

mattermost-build mentioned this pull request Jun 12, 2024

Automated cherry pick of #7953 #8008

Merged

mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jun 12, 2024

cwarnermm added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Jul 11, 2024

Add performance metrics to the app #7953

Add performance metrics to the app #7953

Conversation

larkox commented May 14, 2024 • edited Loading

Summary

Ticket Link

Related PRs

Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum commented May 30, 2024

larkox commented May 30, 2024

larkox commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox commented Jun 11, 2024

yasserfaraazkhan commented Jun 11, 2024

mattermost-build commented Jun 12, 2024

larkox commented May 14, 2024 •

edited

Loading