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

Perf: add support for launch ID #27

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

EdmondChuiHW
Copy link

@EdmondChuiHW EdmondChuiHW commented Mar 20, 2024

Summary

TLDR: We want to keep a stable ID for each launch so we can join CDT sessions.

Open source: no-op as perf metrics is only applicable for internal builds. See #16

Please also see the internal RFC: https://www.internalfb.com/diff/D55164646

This launch ID is read from the query param; thus remains the same for each CDT refresh, either by pressing F5 or clicking "Reconnect DevTools":

const button = createTextButton(i18nString(UIStrings.reconnectDevtools), () => window.location.reload());

Why

Since modern web standards relies on visibilitychange to signal the end of a session, we need to account for users showing and hiding the window multiple times. Closing the window is indistinguishable from hiding the window under this model.

This means each time the window is hidden, we treat this as one completed CDT session and submit end-of-session perf metrics as if the window is closed.

If the user was only hiding the window and came back later, we need a way to de-dup the subsequent events.

Therefore, we need to keep a stable ID across refreshes and visibilitychange events. For each launch, e.g. from iOS, Android, or Metro, a launch ID is generated in the internal version of Metro. See internal diff

We'll then treat all events with the same launch ID as a single session in our metrics analytics.

Pending further discussion, we can also use this launch ID for Metro and CDP backend to attribute perf metrics outside of CDT.

Stack

Stacked on #26

Depended by #28

Preview this PR in isolation at https://github.com/EdmondChuiHW/rn-chrome-devtools-frontend2/pull/1/files

Test plan

Non-null launchId in query param doesn't crash when launched from iOS, Android, and Metro (internal)

Null launchId query param: no-op (for open-source)

Upstreaming plan

  • This commit should be sent as a patch to the upstream devtools-frontend repo. I've reviewed the contribution guide.
  • This commit is React Native-specific and cannot be upstreamed.

@EdmondChuiHW EdmondChuiHW changed the title Add support for launch ID Perf: add support for launch ID Mar 20, 2024
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 21, 2024
Summary:
Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Differential Revision: D55164646
@EdmondChuiHW EdmondChuiHW mentioned this pull request Mar 21, 2024
2 tasks
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 21, 2024
Summary:

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Differential Revision: D55164646
@EdmondChuiHW EdmondChuiHW marked this pull request as ready for review March 21, 2024 01:26
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 21, 2024
Summary:

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Differential Revision: D55164646
@@ -22,6 +22,7 @@ import type * as InspectorBackend from '../../core/protocol_client/InspectorBack

Host.RNPerfMetrics.registerPerfMetricsGlobalPostMessageHandler();

Host.rnPerfMetrics.setLaunchId(Root.Runtime.Runtime.queryParam('launchId'));
Copy link

Choose a reason for hiding this comment

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

Different casing here compared to L23? 🤔

Copy link
Author

@EdmondChuiHW EdmondChuiHW Mar 22, 2024

Choose a reason for hiding this comment

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

Yeah one is static/constructor. One is the global instance. Following the pattern of the upstream UserMetrics:

/** @constructor */
Host.UserMetrics = HostModule.UserMetrics.UserMetrics;
Host.UserMetrics._PanelCodes = HostModule.UserMetrics.PanelCodes;
/** @enum {number} */
Host.UserMetrics.Action = HostModule.UserMetrics.Action;
/** @type {!Host.UserMetrics} */
Host.userMetrics = HostModule.userMetrics;

Can move the first call to become an instance method instead. At first it was a single function, but this repo enforces a namespace-style import so that's why it was put into RNPerfMetrics "static".

front_end/core/host/RNPerfMetrics.ts Show resolved Hide resolved
Copy link

@huntie huntie left a comment

Choose a reason for hiding this comment

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

LGTM :)

@EdmondChuiHW EdmondChuiHW merged commit 2551db8 into facebookexperimental:main Mar 26, 2024
3 checks passed
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 28, 2024
Summary:

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Differential Revision: D55164646
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 28, 2024
Summary:

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Reviewed By: hoxyq

Differential Revision: D55164646
EdmondChuiHW added a commit to EdmondChuiHW/react-native that referenced this pull request Mar 28, 2024
Summary:

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Reviewed By: hoxyq

Differential Revision: D55164646
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 28, 2024
Summary:
Pull Request resolved: #43585

Changelog: [internal]

Related PR: facebookexperimental/rn-chrome-devtools-frontend#27

Reviewed By: hoxyq

Differential Revision: D55164646

fbshipit-source-id: 0f25f150603a24654020093697e76d85d0d8cc02
@EdmondChuiHW EdmondChuiHW mentioned this pull request May 20, 2024
2 tasks
@EdmondChuiHW EdmondChuiHW deleted the launch_id branch May 29, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants