-
Notifications
You must be signed in to change notification settings - Fork 48.7k
[DevTools] add simple usage events for internal logging #24888
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
Conversation
@@ -22,6 +23,7 @@ export default function InspectHostNodesToggle() { | |||
setIsInspecting(isChecked); | |||
|
|||
if (isChecked) { | |||
logEvent({event_name: 'inspect-native-element'}); |
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.
I wonder if this is the best place to put this log, since just because you've clicked the inspect element button doesn't mean you would have selected an element. Maybe we could look this as inspect-element-button-clicked
and have something here to log that the user actually selected something?
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.
How about inspect-element-button-clicked
and inspect-element-success
?
|} | ||
| {| | ||
+event_name: 'select-element-in-tree', | ||
|} | ||
| {| | ||
+event_name: 'select-element-in-owner-view', | ||
|} | ||
| {| | ||
+event_name: 'inspect-native-element', | ||
|} |
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.
Besides the event, is there any other metadata you think we should have? Ex. do we care about which elements were selected?
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.
Yes, I want to discuss with you about adding a "metadata" (maybe not the best name) column to DB. It can be useful for several usage logging (not included in this PR by purpose). But for this one, how do you imagine the additional info about the elements be useful for us?
+event_name: 'select-element-in-tree', | ||
|} | ||
| {| | ||
+event_name: 'select-element-in-owner-view', | ||
|} |
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.
Should we combine the select element methods into one event and use metadata to describe the type of way we select elements, or do you think it's important for all these events to have their own event name?
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 we can combine. I'll change them to the same name now, and add metadata in a follow-up PR. Sounds good?
@@ -103,6 +104,7 @@ export default function Tree(props: Props) { | |||
function handleStopInspectingNative(didSelectNode) { | |||
if (didSelectNode && focusTargetRef.current !== null) { | |||
focusTargetRef.current.focus(); | |||
logEvent({event_name: 'inspect-element-success'}); |
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.
Should this also just be select-element
since it ends up being what it is?
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.
good catch, this should be select-element
with source: 'inspect-element'
Summary
As titled
How did you test this change?
Local build works with internal logging as expected