Add telemetry for reverse search
Categories
(DevTools :: Console, task, P2)
Tracking
(firefox77 fixed)
Tracking | Status | |
---|---|---|
firefox77 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: mroncancio19, Mentored)
References
Details
Attachments
(2 files, 3 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
2.04 KB,
text/plain
|
tdsmith
:
data-review+
|
Details |
We could log when: - the reverse search opens - the reverse search input change - the jsterm text gets evaluated directly from the reverse search (Enter) - navigating results (keyboard or button click)
Reporter | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
List makes sense, I'd be OK to leave out the input change to simplify. Users getting in and out of the worklow is most important.
Comment 3•5 years ago
|
||
Hi Nicolas,
As we discussed, I would like to be assigned to this bug. Thanks.
Reporter | ||
Comment 4•5 years ago
|
||
Hello Erik, I assigned the bug to you, thanks for helping us!
You can read comments on Bug 1531395 where other outreachy applicants are doing a similar task, so you have a sense of what needs to be done and where.
Reporter | ||
Comment 5•5 years ago
|
||
Hello Erik, how is it going on?
Is there anything blocking you on this bug?
Comment hidden (typo) |
Comment 7•5 years ago
|
||
Hello Nicolas,
I have researched the issue at hand and have also looked into the work done by others on Bug 1531395 as you suggested.
In Comment 0 you specify 4 things we want to log using telemetry.
I understand that in the devtools/client/webconsole/middleware/event-telemetry.js
file I have to check for the REVERSE_SEARCH_INPUT_TOGGLE
action type by using an additional else if
statement. In this file I also have to add REVERSE_SEARCH_INPUT_TOGGLE
to the const
at the beginning of the file.
Then in https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/devtools/client/webconsole/actions/ui.js#95
I have to modify the reverseSearchInputToggle()
function.
Where I need some assistance is when I try to log for the last two items from your list on Comment 0:
- the jsterm text gets evaluated directly from the reverse search (Enter)
- navigating results (keyboard or button click)
For this I have looked at https://searchfox.org/mozilla-central/source/devtools/client/webconsole/constants.js#151-161, here I see some declarations for jsterm commands and for the direction of jsterm input history navigation. Is this the right place to look to resolve the last two issues? Thanks.
Reporter | ||
Comment 8•5 years ago
•
|
||
Hello Erik, sorry for the delay!
the jsterm text gets evaluated directly from the reverse search (Enter)
So this is where we react to the <kbd>Enter</kbd> keypress: devtools/client/webconsole/components/ReverseSearchInput.js#89.
You can see we are calling evaluateInput
.
evaluateInput
is passed from devtools/client/webconsole/components/App.js#259, and comes from devtools/client/webconsole/webconsole-wrapper.js#168-170.
As you can see, it only takes oen parameter (the expression we want to evaluate).
What would be nice would be to add a second parameter, which would be from
. So when called from the ReverseSearchInput
, we would pass a second argument (which could be reverse search
for example).
Then we'd need to add a second parameter to devtools/client/webconsole/components/JSTerm.js#586, and default it to console input
for example.
This parameter should then be passed to the requestEvaluation
function (devtools/client/webconsole/components/JSTerm.js#660-671, where we can use it in the telemetry call
this.props.serviceContainer.recordTelemetryEvent("execute_js", {
"lines": str.split(/\n/).length,
from,
});
navigating results (keyboard or button click)
So these are the actions that are dispatched to navigate devtools/client/webconsole/actions/history.js#72,78.
You can target these 2 actions in the middleware. The only thing we're missing here is how these functions are called (mouse click or key press).
So we could add a parameter to those functions as well (we could use from
, again), and pass either keyboard
or button
when calling them in devtools/client/webconsole/components/ReverseSearchInput.js#126,181.
Let me know if I'm not clear enough
Reporter | ||
Comment 9•5 years ago
|
||
Hello Erik, do you still plan to work on this? It's totally fine if not :)
Reporter | ||
Comment 10•5 years ago
|
||
clearing assignee
Assignee | ||
Comment 11•5 years ago
|
||
Hi I would like to work on this issue, but I might need some guidance.
Reporter | ||
Comment 12•5 years ago
|
||
Hello Miguel,
Thank you for helping!
To summarize, we want to know how our reverse search feature is used.
Reverse search can be toggled in the console using F9 (or Ctrl + R in OSX).
There are a few ways a user can show the reverse search:
- By clicking the dedicated button in the Editor Toolbar devtools/client/webconsole/components/Input/EditorToolbar.js#47-49
- With F9 (or Ctrl+R on OSX) : devtools/client/webconsole/components/App.js#124
You may have noticed that the webconsole is a React/Redux app. That makes it possible to listen for all the actions being dispatched in a middleware. Luckily we already have one where we record telemetry events: devtools/client/webconsole/middleware/event-telemetry.js
So in devtools/client/webconsole/middleware/event-telemetry.js#35-62 , we need to add another else if
block that check if the action is REVERSE_SEARCH_INPUT_TOGGLE
(defined in devtools/client/webconsole/constants.js#36 ).
We also need to add a new event entry in toolkit/components/telemetry/Events.yaml#1093-1108(you can copy paste the linked entry and change the names).
let me know if everything is okay for now, and then we can discuss further actions. You can also join us on Slack if you have questions :)
Assignee | ||
Comment 13•5 years ago
|
||
Hi Nicolas,
I have edited the event-telemetry.js file with the following:
At imports:
const {
FILTER_TEXT_SET,
FILTER_TOGGLE,
DEFAULT_FILTERS_RESET,
EVALUATE_EXPRESSION,
MESSAGES_ADD,
PERSIST_TOGGLE,
REVERSE_SEARCH_INPUT_TOGGLE // added
} = require("devtools/client/webconsole/constants");
and inside eventTelemetryMiddleware function:
else if (action.type === REVERSE_SEARCH_INPUT_TOGGLE){
telemetry.recordEvent(
"reverse_search_toggle",
"webconsole",
null,
{
access: action.accessOrigin,
session_id: sessionId,
},
);
}
The changes are reflected in actions/ui.js:
function reverseSearchInputToggle({ initialValue, access } = {}) {
return {
type: REVERSE_SEARCH_INPUT_TOGGLE,
initialValue,
accessOrigin: access,
};
}
inside the components/App.js onKeyDown handler:
onKeyDown(event) {
const { dispatch, webConsoleUI, editorFeatureEnabled } = this.props;
if (
(!isMacOS && event.key === "F9") ||
(isMacOS && event.key === "r" && event.ctrlKey === true)
) {
const initialValue =
webConsoleUI.jsterm && webConsoleUI.jsterm.getSelectedText();
const access = isMacOS ? `keyboard_shortcut: Ctrl+${event.key}` : event.key; // added
dispatch(actions.reverseSearchInputToggle({ initialValue, access })); // modified
event.stopPropagation();
}
if (
editorFeatureEnabled &&
event.key.toLowerCase() === "b" &&
((isMacOS && event.metaKey) || (!isMacOS && event.ctrlKey))
) {
event.stopPropagation();
event.preventDefault();
dispatch(actions.editorToggle());
}
}
and finally in components/Input/EditorToolbar.js
onReverseSearchButtonClick(event) {
const { dispatch, serviceContainer } = this.props;
event.stopPropagation();
dispatch(
actions.reverseSearchInputToggle({
initialValue: serviceContainer.getInputSelection(),
access: "Editor Toolbar",
})
);
}
This is the entry I added after execute_js in the Events.yaml file:
reverse_search_toggle:
objects: ["webconsole"]
bug_numbers: [1463083] # does the bug number need to go here?
notification_emails: ["dev-developer-tools@lists.mozilla.org", "hkirschner@mozilla.com"]
products:
- "firefox"
- "fennec"
- "geckoview"
record_in_processes: ["main"]
description: User has toggled reverse-search.
release_channel_collection: opt-out
expiry_version: never
extra_keys:
access: Indicates where reverse search was accessed from (keyboard or toolbar)
session_id: The start time of the session in milliseconds since epoch (Unix Timestamp) e.g. 1396381378123.
I have a couple of questions:
-
Is creating an extra
access
key in the event entry is necessary? Does it makes more sense to use thevalue
argument of the telemetry.recordEvent method? -
should the bug number for this issue be entered in the event entry? I.e:
bug_numbers: [1489489]
?
Let me know if I am on the right track or need to make some changes.
Comment 14•5 years ago
|
||
Setting needinfo flag on nchevobbe for comment 13
Reporter | ||
Comment 15•5 years ago
|
||
Thank you for your patience Miguel
(In reply to Miguel Roncancio from comment #13)
I have a couple of questions:
- Is creating an extra
access
key in the event entry is necessary? Does it makes more sense to use thevalue
argument of the telemetry.recordEvent method?
I think it might be better to use value
indeed.
- should the bug number for this issue be entered in the event entry? I.e:
bug_numbers: [1489489]
?
Yes :)
Let me know if I am on the right track or need to make some changes.
You're definitely on the right track. One thing though: we might want to only record reverse search opening, not closing.
To do that, in event-telemetry.js we might want to check the app state to know if we're opening or closing.
Let me know if you have other questions, and if you do, use the "needinfo" checkbox which is below the comment textarea :)
Assignee | ||
Comment 16•4 years ago
|
||
Assignee | ||
Comment 17•4 years ago
|
||
Assignee | ||
Comment 18•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 19•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 20•4 years ago
|
||
Hey @nchevobbe, I've been trying to figure out how to go about closing the reverse search input when the user evaluates something from the reverse search input but haven't had much success. I noticed that there is an EventEmitter under webconsole.js. I thought I could use this to emit a key event and close the reverse search that way but I am unsure if this is the right way to go about it. I also am not sure as to how to use this emitter. help?
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 21•4 years ago
|
||
Hello Miguel,
So here's the handler for the <kbd>Enter</kbd>: devtools/client/webconsole/components/Input/ReverseSearchInput.js#97
It calls devtools/client/webconsole/actions/input.js#59-104
So we could modify this action to accept a second argument, which would be from
and that would default to input
.
We would pass this from
value, into the object we dispatch (devtools/client/webconsole/actions/input.js#77-80 )
In the reverse search component, we could call dispatch(actions.evaluateExpression(undefined, "reverse-search"));
And finally, in the telemetry middleware , we could check this from
property and act accordingly.
What do you think?
Reporter | ||
Comment 22•4 years ago
|
||
Hello Miguel,
Can I help you with something? Are you blocked?
Maybe you don't have the time to look into this and it's totally fine! Let me know and I'll unassign you.
Assignee | ||
Comment 23•4 years ago
|
||
Hi Nicolas,
Sorry for not getting back to you earlier! I tried what you suggested and though it is a cleaner implementation, it did not solve the issue (closing the reverse search after evaluate expression). At this point I think I have to check all the changes I made and determine which one it is that is causing this behavior. I would like to continue to work on this and see it through but I don't know that I will have time until mid February or March :/
Reporter | ||
Comment 24•4 years ago
|
||
No worries at all, this is fine :)
I'll keep the bug assigned to you and will check in later.
Reporter | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Comment 25•4 years ago
|
||
harald, do you know who we should ask for data review? looks like the patch is ready to land :)
Updated•4 years ago
|
Comment 26•4 years ago
|
||
Comment 27•4 years ago
|
||
Comment on attachment 9140122 [details] request.md 1) Is there or will there be **documentation** that describes the schema for the ultimate data set in a public, complete, and accurate way? Yes, in Events.yaml and the probe dictionary. 2) Is there a control mechanism that allows the user to turn the data collection on and off? Yes, the Firefox telemetry opt-out. 3) If the request is for permanent data collection, is there someone who will monitor the data over time? Yes, Harald. 4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under? Category 2, interaction data. 5) Is the data collection request for default-on or default-off? Default-on. 6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? No. 7) Is the data collection covered by the existing Firefox privacy notice? Yes. 8) Does there need to be a check-in in the future to determine whether to renew the data? No, permanent collection. 9) Does the data collection use a third-party collection tool? No; Amplitude for viz, but using Mozilla infrastructure.
Comment 28•4 years ago
|
||
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ab7d64c5c50c add reverse search telemetry. r=nchevobbe.
Comment 29•4 years ago
|
||
bugherder |
Description
•