[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API #3165

devinbinnie · 2024-10-10T17:18:26Z

Summary

This PR contains a new module called performanceMonitor, which is responsible for:

Collect metrics from the main process and various utility processes
Dispatch requests to the renderer processes for their respective metrics
Collect all metrics and make any transformations necessary
Send those metrics to the servers that are able to accept and display them at a set interval

This monitoring will take CPU and memory usage data and send it to servers that have elected to subscribe to the Desktop App for performance data. This data will then be available to administrators via Prometheus.

The idea behind this is to get a better idea of how hard users are getting hit in terms of system resources by various parts of our app. The data is divided up by unique process, so that should give us some idea of which part of the app is causing issues if there is a lot of system resources being dedicated to a process.

Not in scope for this change are load times, which we will calculate and send as part of a separate API created later.

Ticket Link

https://mattermost.atlassian.net/browse/MM-60610
https://mattermost.atlassian.net/browse/MM-60086

Screenshots

Implement a performanceMonitor to collect and send anonymous usage data to server dashboards.

… usage data and send via API

github-actions · 2024-10-11T05:05:08Z

Here are the test results below:

Test Summary for Linux on commit `2679017`

New failed tests found on Linux:

menu/view MM-T816 Toggle Full Screen in the Menu Bar

Test Summary for macOS on commit `2679017`

All stable tests passed on macOS.

yasserfaraazkhan · 2024-10-11T15:56:59Z

verified e2e tests locally. No blocking failures.

pvev

LGTM! thanks @devinbinnie

pvev · 2024-10-13T22:03:47Z

src/main/performanceMonitor.ts

+                view.webContents.send(METRICS_REQUEST, view.name, view.serverId);
+            });
+        });
+        await Promise.all(viewPromises);


maybe here would be good to add some preventive code:

use allSettled instead of all so you can handle individual promise rejections (in case one of the promise rejects).

would be good to wrap this line and the following in a try..finally, so you can make sure that no matter what the removal of the listener will happen.

should there be a timeout maybe?

i don't see a case where I wouldn't want the app to be loud about the Promise failing, so the try/catch doesn't seem as necessary. However I did want to add the timeout, thank you for reminding me :)

pvev · 2024-10-13T22:04:50Z

src/main/performanceMonitor.ts

+            }
+
+            const serverMetricsMap = new Map([...metricsMap].filter((value) => !value[1].serverId || value[1].serverId === view.serverId));
+            view.webContents?.send(METRICS_SEND, serverMetricsMap);


no need for optional chaining view.webContents?.send since it occurs after checking if (!view.webContents)

pvev · 2024-10-13T22:17:37Z

src/main/views/serverDropdownView.ts

@@ -83,6 +84,7 @@ export class ServerDropdownView {
            // @ts-ignore
            transparent: true,
        }});
+        performanceMonitor.registerView('ServerDropdownView', this.view.webContents);


nit: for this and all the other registered views, maybe put the names in global constant values.

Suggested change

performanceMonitor.registerView('ServerDropdownView', this.view.webContents);

performanceMonitor.registerView(SERVER_DROPDOWN_VIEW, this.view.webContents);

This would be worth doing I'd say if we were using these constants in multiple places, but right now they're just labels that get passed along with the data and will only matter once it hits the dashboard, so I'm okay to leave this one as is.

devinbinnie · 2024-10-18T14:14:31Z

@amyblais Am I okay to cherry-pick this to v5.10 to include in the next RC?

amyblais · 2024-10-18T14:33:24Z

Am I okay to cherry-pick this to v5.10 to include in the next RC?

~~I think this would be better for v5.11 unless this is considered to be urgent for v5.10. I thought you were planning only bug fixes for RC-2.~~ We are cherry-picking to v5.10

devinbinnie · 2024-10-18T14:38:01Z

/cherry-pick release-5.10

mattermost-build · 2024-10-18T14:38:04Z

Cherry pick is scheduled.

… usage data and send via API (#3165) * [MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API * Translations * PR feedback * Update api-types package (cherry picked from commit 1029516)

… usage data and send via API (#3165) (#3171) * [MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API * Translations * PR feedback * Update api-types package (cherry picked from commit 1029516) Co-authored-by: Devin Binnie <[email protected]>

devinbinnie added 2 commits October 10, 2024 13:04

[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory…

1f6d17b

… usage data and send via API

Translations

2679017

devinbinnie added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester 1: UX Review Requires review by a UX Designer labels Oct 10, 2024

devinbinnie requested review from larkox, pvev, abhijit-singh and yasserfaraazkhan October 10, 2024 17:18

mm-cloud-bot added the release-note label Oct 10, 2024

yasserfaraazkhan added the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 11, 2024

github-actions bot removed the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 11, 2024

larkox approved these changes Oct 11, 2024

View reviewed changes

yasserfaraazkhan added QA Deferred and removed 3: QA Review Requires review by a QA tester labels Oct 11, 2024

yasserfaraazkhan approved these changes Oct 11, 2024

View reviewed changes

pvev approved these changes Oct 13, 2024

View reviewed changes

devinbinnie removed the 2: Dev Review Requires review by a core committer label Oct 15, 2024

devinbinnie added 2 commits October 15, 2024 13:25

PR feedback

92222af

Update api-types package

bcdb09d

devinbinnie mentioned this pull request Oct 16, 2024

[MM-60609][MM-60612] Include Desktop App metrics in PerformanceReporter, add metrics in Prometheus for CPU/Memory usage mattermost/mattermost#28825

Open

abhijit-singh added the Build Apps for PR Builds signed builds for testing label Oct 18, 2024

abhijit-singh approved these changes Oct 18, 2024

View reviewed changes

devinbinnie merged commit 1029516 into master Oct 18, 2024
19 of 20 checks passed

devinbinnie deleted the MM-60086_MM-60610 branch October 18, 2024 14:13

devinbinnie added CherryPick/Candidate A candidate for a quality or patch release, but not yet approved and removed 1: UX Review Requires review by a UX Designer labels Oct 18, 2024

devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed Build Apps for PR Builds signed builds for testing labels Oct 18, 2024

amyblais added this to the v5.10.0 milestone Oct 18, 2024

amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Oct 18, 2024

mattermost-build mentioned this pull request Oct 18, 2024

Automated cherry pick of #3165 #3171

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 Oct 18, 2024

amyblais added the Docs/Needed label Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API #3165

[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API #3165

devinbinnie commented Oct 10, 2024

github-actions bot commented Oct 11, 2024

yasserfaraazkhan commented Oct 11, 2024

pvev left a comment

pvev Oct 13, 2024

devinbinnie Oct 15, 2024

pvev Oct 13, 2024

pvev Oct 13, 2024

devinbinnie Oct 15, 2024

devinbinnie commented Oct 18, 2024

amyblais commented Oct 18, 2024 •

edited

Loading

devinbinnie commented Oct 18, 2024

mattermost-build commented Oct 18, 2024

	performanceMonitor.registerView('ServerDropdownView', this.view.webContents);
	performanceMonitor.registerView(SERVER_DROPDOWN_VIEW, this.view.webContents);

[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API #3165

[MM-60086][MM-60610] Implement performanceMonitor, collect CPU/memory usage data and send via API #3165

Conversation

devinbinnie commented Oct 10, 2024

Summary

Ticket Link

Screenshots

github-actions bot commented Oct 11, 2024

Test Summary for Linux on commit 2679017

Test Summary for macOS on commit 2679017

yasserfaraazkhan commented Oct 11, 2024

pvev left a comment

Choose a reason for hiding this comment

pvev Oct 13, 2024

Choose a reason for hiding this comment

devinbinnie Oct 15, 2024

Choose a reason for hiding this comment

pvev Oct 13, 2024

Choose a reason for hiding this comment

pvev Oct 13, 2024

Choose a reason for hiding this comment

devinbinnie Oct 15, 2024

Choose a reason for hiding this comment

devinbinnie commented Oct 18, 2024

amyblais commented Oct 18, 2024 • edited Loading

devinbinnie commented Oct 18, 2024

mattermost-build commented Oct 18, 2024

Test Summary for Linux on commit `2679017`

Test Summary for macOS on commit `2679017`

amyblais commented Oct 18, 2024 •

edited

Loading