-
Notifications
You must be signed in to change notification settings - Fork 8
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
Perf: add support for launch ID #27
Conversation
Summary: Changelog: [internal] Related PR: facebookexperimental/rn-chrome-devtools-frontend#27 Differential Revision: D55164646
Summary: Changelog: [internal] Related PR: facebookexperimental/rn-chrome-devtools-frontend#27 Differential Revision: D55164646
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')); |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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
:
rn-chrome-devtools-frontend/front_end/core/host/host-legacy.ts
Lines 39 to 48 in 9370b35
/** @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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Summary: Changelog: [internal] Related PR: facebookexperimental/rn-chrome-devtools-frontend#27 Differential Revision: D55164646
Summary: Changelog: [internal] Related PR: facebookexperimental/rn-chrome-devtools-frontend#27 Reviewed By: hoxyq Differential Revision: D55164646
Summary: Changelog: [internal] Related PR: facebookexperimental/rn-chrome-devtools-frontend#27 Reviewed By: hoxyq Differential Revision: D55164646
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
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":
rn-chrome-devtools-frontend/front_end/ui/legacy/RemoteDebuggingTerminatedScreen.ts
Line 47 in 9370b35
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 diffWe'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
devtools-frontend
repo. I've reviewed the contribution guide.