[DevTools] RFC: log profiling events under __PROFILE__ flag #22167
Conversation
| @@ -10,6 +10,9 @@ | |||
| // Flip this flag to true to enable verbose console debug logging. | |||
| export const __DEBUG__ = false; | |||
|
|
|||
| // Flip this flag to true to enable performance logging to console. | |||
| export const __PROFILE__ = false; | |||
There was a problem hiding this comment.
Don't have time to review this full PR now, but a quick thought: The __PROFILE__ flag has a specific meaning for React builds already. (There's even a global __PROFILE__ flag defined for DevTools too, in the Webpack configs.)
I think this functionality is a bit different. Maybe we should use a different name to avoid confusion?
|
oops, a few unrelated changes got added to this PR (some refactoring of the calls to |
| import type {SourceConsumer} from '../astUtils'; | ||
|
|
||
| const MAX_SOURCE_LENGTH = 100_000_000; | ||
|
|
||
| if (__PROFILE__) { | ||
| global.__loggerEvents = []; |
There was a problem hiding this comment.
I'm wondering why we are using a global here?
Seems like we could use our feature flag system to create a proper logging (enabled for Facebook builds only) that could batch and flush these to some internal logging system.
There was a problem hiding this comment.
yeah the global was just for manual testing purposes. we don't have to land this global, but i would imagine as a follow up we would do this:
Seems like we could use our feature flag system to create a proper logging (enabled for Facebook builds only) that could batch and flush these to some internal logging system.
There was a problem hiding this comment.
Fair enough! Works fine for a POC 😁
I think I'd probably want to do this design work as part of the initial PR rather than a follow up though.
| @@ -104,12 +128,73 @@ const originalURLToMetadataCache: LRUCache< | |||
| }, | |||
| }); | |||
|
|
|||
| function withSyncProfiling<TReturn>( | |||
There was a problem hiding this comment.
This function and the withCallbackProfiling function below could be part of such a logger. (Maybe non-Facebook builds would just get a no-op logger that does nothing.)
|
I forwarded the mail to the wrong group. I'm sorry from the bottom of my
heart
เมื่อ พ. 25 ส.ค. 2564 เวลา 05:58 Brian Vaughn ***@***.***>
เขียนว่า:
***@***.**** commented on this pull request.
------------------------------
In packages/react-devtools-extensions/src/parseHookNames/parseHookNames.js
<#22167 (comment)>:
> @@ -104,12 +128,73 @@ const originalURLToMetadataCache: LRUCache<
},
});
+function withSyncProfiling<TReturn>(
This function and the withCallbackProfiling function below could be part
of such a logger. (Maybe non-Facebook builds would just get a no-op logger
that does nothing.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#22167 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJGPVI7K35WEMAAD6S57Z53T6QP2XANCNFSM5CXLALTA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
--
I-Phone.Su
|
|
abandoning in favor of #22276 |
Summary
This commit is being used as proposal for adding a logger (
EventLogger) that records events happening in the DevTools runtime. In particular, this code was used for collecting profiling data to measure the performance ofparseHookNames.The current
EventLoggercan be extended to log and collect different types of events that we can statically enforce with Flow (e.g. for performance metrics, or for other relevant product measurements), and which can then be accessed by various means. Right now, they are just stored in a global constant, but in the future they could be logged to other logging systems (e.g. external logging systems).Note that when
__PROFILE__flag is disabled, all of the event logging will be disabled and stripped from the build.Test Plan