Closed Bug 1489489 Opened 6 years ago Closed 4 years ago

Add telemetry for reverse search

Categories

(DevTools :: Console, task, P2)

task

Tracking

(firefox77 fixed)

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: nchevobbe, Assigned: mroncancio19, Mentored)

References

Details

Attachments

(2 files, 3 obsolete files)

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)
any thoughts Harald ?
Flags: needinfo?(hkirschner)
Priority: -- → P2
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.
Flags: needinfo?(hkirschner)

Hi Nicolas,

As we discussed, I would like to be assigned to this bug. Thanks.

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.

Assignee: nobody → ecarr3344
Mentor: nchevobbe
Status: NEW → ASSIGNED

Hello Erik, how is it going on?
Is there anything blocking you on this bug?

Flags: needinfo?(ecarr3344)

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.

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

Hello Erik, do you still plan to work on this? It's totally fine if not :)

Flags: needinfo?(ecarr3344)

clearing assignee

Assignee: ecarr3344 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ecarr3344)

Hi I would like to work on this issue, but I might need some guidance.

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:

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: nobody → mroncancio19
Status: NEW → ASSIGNED

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:

  1. Is creating an extraaccess key in the event entry is necessary? Does it makes more sense to use the value argument of the telemetry.recordEvent method?

  2. 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.

Setting needinfo flag on nchevobbe for comment 13

Flags: needinfo?(nchevobbe)

Thank you for your patience Miguel

(In reply to Miguel Roncancio from comment #13)

I have a couple of questions:

  1. Is creating an extraaccess key in the event entry is necessary? Does it makes more sense to use the value argument of the telemetry.recordEvent method?

I think it might be better to use value indeed.

  1. 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 :)

Flags: needinfo?(nchevobbe)
Attachment #9107616 - Attachment is obsolete: true
Attachment #9108913 - Attachment description: Bug 1489489 - add reverse search telemetry → Bug 1489489 - Add telemetry for WebConsole reverse search feature. r=nchevobbe.
Attachment #9108913 - Attachment description: Bug 1489489 - Add telemetry for WebConsole reverse search feature. r=nchevobbe. → Bug 1489489 - add reverse search telemetry
Attachment #9108913 - Attachment description: Bug 1489489 - add reverse search telemetry → Bug 1489489 - Add telemetry for WebConsole reverse search feature. r=nchevobbe
Attachment #9108913 - Attachment description: Bug 1489489 - Add telemetry for WebConsole reverse search feature. r=nchevobbe → Bug 1489489 - add reverse search telemetry

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?

Flags: needinfo?(nchevobbe)

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?

Flags: needinfo?(nchevobbe)

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.

Flags: needinfo?(mroncancio19)

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 :/

Flags: needinfo?(mroncancio19)

No worries at all, this is fine :)
I'll keep the bug assigned to you and will check in later.

Type: enhancement → task
Attachment #9108855 - Attachment is obsolete: true
Attachment #9113272 - Attachment is obsolete: true

harald, do you know who we should ask for data review? looks like the patch is ready to land :)

Flags: needinfo?(hkirschner)
Attachment #9108913 - Attachment description: Bug 1489489 - add reverse search telemetry → Bug 1489489 - add reverse search telemetry. r=nchevobbe.
Attached file request.md
Flags: needinfo?(hkirschner)
Attachment #9140122 - Flags: data-review?(tdsmith)
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.
Attachment #9140122 - Flags: data-review?(tdsmith) → data-review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab7d64c5c50c
add reverse search telemetry. r=nchevobbe.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
You need to log in before you can comment on or make changes to this bug.