Network metrics #8390

enahum · 2024-12-03T01:01:45Z

Summary

This PR allows us to collect network metrics for analysis.

Ticket Link

https://mattermost.atlassian.net/browse/MM-61878

Release Note

NONE

rahimrahman · 2024-12-09T21:11:20Z

Hi @enahum -- sorry for the delay on this.

I was able to see the label showing up in the log:

 DEBUG  Group "entry" completed.
 LOG  Telemetry event on ios for server https://community.mattermost.com
            Group "entry"
            requesting 35 urls
            total Compressed size of: 0 B
            total size of: 0 B
            ellapsed time: 4.697 seconds
            average latency: 0 ms
 DEBUG  Group "entry-additional" completed.
 LOG  Telemetry event on ios for server https://community.mattermost.com
            Group "entry-additional"
            requesting 16 urls
            total Compressed size of: 0 B
            total size of: 0 B
            ellapsed time: 0.607 seconds
            average latency: 0 ms

-----

 LOG  Telemetry event on ios for server https://community.mattermost.com
            Group "reconnection"
            requesting 32 urls
            total Compressed size of: 0 B
            total size of: 0 B
            ellapsed time: 0.972 seconds
            average latency: 0 ms
 DEBUG  Group "reconnection-additional" completed.
 LOG  Telemetry event on ios for server https://community.mattermost.com
            Group "reconnection-additional"
            requesting 16 urls
            total Compressed size of: 0 B
            total size of: 0 B
            ellapsed time: 0.505 seconds
            average latency: 0 ms

Is the total size suppose to be 0B and latency at 0ms? I am using "github:mattermost/react-native-network-client#ad7b88412f41c5a1024420a4f4c7461883cd0e63" as in package.json. This is my way to test react-native-network-client changes as well. Did I miss anything?

My reluctant is the amount of changes involved to make this work. Is it because groupLabel can change at anytime (similar to serverUrl)? (as portrayed here where you can have "entry" and "entry-additional" all within one single lifecycle).

isacikgoz

Can't tell anything specific to implementation but the metrics seems okay to me. Looking forward to analyse these.

enahum · 2024-12-09T23:57:48Z

Hi @enahum -- sorry for the delay on this.

I was able to see the label showing up in the log:
 DEBUG  Group "entry" completed.

 LOG  Telemetry event on ios for server https://community.mattermost.com

            Group "entry"

            requesting 35 urls

            total Compressed size of: 0 B

            total size of: 0 B

            ellapsed time: 4.697 seconds

            average latency: 0 ms

 DEBUG  Group "entry-additional" completed.

 LOG  Telemetry event on ios for server https://community.mattermost.com

            Group "entry-additional"

            requesting 16 urls

            total Compressed size of: 0 B

            total size of: 0 B

            ellapsed time: 0.607 seconds

            average latency: 0 ms



-----



 LOG  Telemetry event on ios for server https://community.mattermost.com

            Group "reconnection"

            requesting 32 urls

            total Compressed size of: 0 B

            total size of: 0 B

            ellapsed time: 0.972 seconds

            average latency: 0 ms

 DEBUG  Group "reconnection-additional" completed.

 LOG  Telemetry event on ios for server https://community.mattermost.com

            Group "reconnection-additional"

            requesting 16 urls

            total Compressed size of: 0 B

            total size of: 0 B

            ellapsed time: 0.505 seconds

            average latency: 0 ms
Is the total size suppose to be 0B and latency at 0ms? I am using "github:mattermost/react-native-network-client#ad7b88412f41c5a1024420a4f4c7461883cd0e63" as in package.json. This is my way to test react-native-network-client changes as well. Did I miss anything?

My reluctant is the amount of changes involved to make this work. Is it because groupLabel can change at anytime (similar to serverUrl)? (as portrayed here where you can have "entry" and "entry-additional" all within one single lifecycle).

@rahimrahman you seem to not have set the collectMetrics config setting to true, not sure how you missed that.

As for the changes, the groupLabel is applied in multiple places as we need to know what endpoints to group. For example you'll see entry and entry-additional in this PR but in a future PR I have almost ready to go it will change to the following labels (this labels will be well defined in a type definition):

Login & Login Deferred
Cold Start & Cold Start Deferred
Notification & Notification Deferred
DeepLink & DeepLink Deferred
WebSocket reconnect & WebSocket reconnect Deferred

Why the Deferred? Simply because I thought is better to group them in a way that we can clearly identify the requests that are done for TTI and those that happen after like fetching posts for unread channels threads and in case of this PR also channels for other teams, etc.. but are still part of the initial load.

Then we can use the groupLabel to group all the requests that are involved in a particular action like channel switch or post a message or whatever

rahimrahman · 2024-12-10T03:14:08Z

Oh shoot, I changed the DEFAULT_CONFIG, and didn't change my LocalConfig.

Silly me, thanks!

rahimrahman

Again, sorry for the tardiness. Here's a few thoughts.

rahimrahman · 2024-12-10T16:39:36Z

app/client/rest/tracking.ts

+    completionTimer?: NodeJS.Timeout;
+}
+
+export default class ClientTraking {


Spelling error

Suggested change

export default class ClientTraking {

export default class ClientTracking {

rahimrahman · 2024-12-10T17:00:56Z

app/client/rest/tracking.ts

+            requesting ${urls.length} urls 
+            total Compressed size of: ${getFormattedFileSize(groupData.totalCompressedSize)}
+            total size of: ${getFormattedFileSize(groupData.totalSize)}
+            ellapsed time: ${duration / 1000} seconds


Suggested change

ellapsed time: ${duration / 1000} seconds

elapsed time: ${duration / 1000} seconds

rahimrahman · 2024-12-10T17:38:15Z

app/client/rest/tracking.ts

+    count: number;
+    metrics?: ClientResponseMetrics;
+}
+


Can we add:

export enum GroupLabel { Entry = 'Entry', ColdStart = 'ColdStart', Login = 'Login', ... }

So we can use it in methods

WebsocketManager.openAll(GroupLabel.Entry);

export const fetchGroupsForChannel = async (serverUrl: string, channelId: string, fetchOnly = false, groupLabel?: GroupLabel) => {

This is done in a future PR as I mentioned in my previous comment, so for this PR it will remain as is

For example you'll see entry and entry-additional in this PR but in a future PR I have almost ready to go it will change to the following labels (this labels will be well defined in a type definition):

Thanks, I missed it.

rahimrahman · 2024-12-10T20:04:58Z

app/client/rest/tracking.ts

+        if (group.urls[url]) {
+            group.urls[url].count += 1;
+        } else {
+            group.urls[url] = {
+                count: 1,
+                metrics,
+            };
+        }


What do you think about using just the path instead of the entire url?

Maybe it will give us more duplicates if any?

LOG {"path": "/api/v4/users/me/preferences", "query": undefined} LOG {"path": "/api/v4/users/me/teams", "query": undefined} LOG {"path": "/api/v4/users/me/teams/members", "query": undefined} LOG {"path": "/api/v4/users/me", "query": undefined} LOG {"path": "/api/v4/users/me/status", "query": undefined} LOG {"path": "/api/v4/users/me/teams/rcgiyftm7jyrxnma1osd8zswby/channels/members", "query": undefined} LOG {"path": "/api/v4/users/me/teams/rcgiyftm7jyrxnma1osd8zswby/channels", "query": "include_deleted=true&last_delete_at=1733860858455"} LOG {"path": "/api/v4/users/me/teams/rcgiyftm7jyrxnma1osd8zswby/channels/categories", "query": undefined}

Suggested change

if (group.urls[url]) {

group.urls[url].count += 1;

} else {

group.urls[url] = {

count: 1,

metrics,

};

}

const [path] = url.split('?');

if (group.urls[path]) {

group.urls[path].count += 1;

} else {

group.urls[path] = {

count: 1,

metrics,

};

}

They would not be really duplicates though.. I think at this time we can keep the url and if we want to try and refine more then we can do soemething like this

Network metrics

a8fe5c8

enahum added the 2: Dev Review Requires review by a core commiter label Dec 3, 2024

enahum requested review from isacikgoz and rahimrahman December 3, 2024 01:01

mm-cloud-bot added release-note-none labels Dec 3, 2024

enahum added 4 commits December 3, 2024 09:10

update network-client ref

babd9d4

fix unit tests

9dddad3

missing catch error parsing

823d8d6

add client tracking unit tests

18fe1c9

isacikgoz reviewed Dec 9, 2024

View reviewed changes

rahimrahman reviewed Dec 10, 2024

View reviewed changes

enahum requested a review from rahimrahman December 10, 2024 21:56

rahimrahman approved these changes Dec 10, 2024

View reviewed changes

fix typos

2553817

enahum merged commit fb57c42 into main Dec 17, 2024
6 checks passed

enahum deleted the request-metrics branch December 17, 2024 11:51

amyblais added this to the v2.25.0 milestone Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network metrics #8390

Network metrics #8390

enahum commented Dec 3, 2024

rahimrahman commented Dec 9, 2024

isacikgoz left a comment

enahum commented Dec 9, 2024

rahimrahman commented Dec 10, 2024

rahimrahman left a comment

rahimrahman Dec 10, 2024

rahimrahman Dec 10, 2024

rahimrahman Dec 10, 2024

enahum Dec 10, 2024

rahimrahman Dec 10, 2024

rahimrahman Dec 10, 2024

enahum Dec 10, 2024

	export default class ClientTraking {
	export default class ClientTracking {

	ellapsed time: ${duration / 1000} seconds
	elapsed time: ${duration / 1000} seconds

Network metrics #8390

Network metrics #8390

Conversation

enahum commented Dec 3, 2024

Summary

Ticket Link

Release Note

rahimrahman commented Dec 9, 2024

isacikgoz left a comment

Choose a reason for hiding this comment

enahum commented Dec 9, 2024

rahimrahman commented Dec 10, 2024

rahimrahman left a comment

Choose a reason for hiding this comment

rahimrahman Dec 10, 2024

Choose a reason for hiding this comment

rahimrahman Dec 10, 2024

Choose a reason for hiding this comment

rahimrahman Dec 10, 2024

Choose a reason for hiding this comment

enahum Dec 10, 2024

Choose a reason for hiding this comment

rahimrahman Dec 10, 2024

Choose a reason for hiding this comment

rahimrahman Dec 10, 2024

Choose a reason for hiding this comment

enahum Dec 10, 2024

Choose a reason for hiding this comment