1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: data-review.txt

Events.yaml:
devtools.main:
  filter_changed:
    objects: ["webconsole"],
    bug_numbers: [1463095]
    notification_emails: ["[e-mail]", "[e-mail]"]
    record_in_processes: ["main"]
    description: User has changed a filter in the web console.
    release_channel_collection: opt-out
    expiry_version: never
    extra_keys:
      clicked: "The clicked filter: error, warn, log, info, debug, css, netxhr or net"
      active: Comma separated list of active filters.
      inactive: Comma separated list of inactive filters.

Usage:
this.telemetry.recordEvent("devtools.main", "filter_changed", "webconsole", null, {
  clicked: "css"
  active: "error,warn,log,css"
  inactive: "info,debug,netxhr,net"
});

Comment 2

[François Marier [:francois]]

Comment on attachment 8979205 [details]
data-review.txt

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, Events.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Harald Kirschner.

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

5) Is the data collection request for default-on or default-off?

Default on, all channels.

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.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

There are also a couple of telemetry.md changes that are not worth splitting out into a different bug.

Review commit: https://reviewboard.mozilla.org/r/255294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255294/

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8990257 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

https://reviewboard.mozilla.org/r/255294/#review262172

Thanks for the patch Mike, I will forward to Nicolas though.
I tried to comment on everything that seemed off, but I'm not involved with the webconsole enough to perform the final review here.

::: devtools/client/webconsole/actions/filters.js:10
(Diff revision 1)
> +const Telemetry = require("devtools/client/shared/telemetry");
> +const telemetry = new Telemetry();

I am wondering if doing telemetry things here will not break console mocha tests. Should check with Nicolas.

::: devtools/client/webconsole/actions/filters.js:39
(Diff revision 1)
> +                         .filter(key => key !== "text" ? filterState[key] : "");
> +    const inactive = Object.keys(filterState)
> +                           .filter(key => key !== "text" ? !filterState[key] : "");

Why are we treating "text" differently? Shouldn't this return false in this case rather than ""?

::: devtools/client/webconsole/components/FilterButton.js:33
(Diff revision 1)
> -      dispatch(actions.filterToggle(filterKey));
> +      const frame = event.target.ownerGlobal.frameElement;
> +      let sessionId = -1;
> +
> +      if (frame && frame.ownerGlobal.frameElement) {
> +        sessionId = frame.ownerGlobal.frameElement.getAttribute("session_id");
> +      }
> +
> +      dispatch(actions.filterToggle(filterKey, sessionId));

I think the added logic should be moved to the "serviceContainer" that is used throughout the webconsole. Something like "getTelemetrySessionId".

Some comments to explain why we expect the attribute to be on `event.target.ownerGlobal.frameElement.ownerGlobal.frameElement` would also help.

Probably better if Nicolas can weigh in.

::: devtools/client/webconsole/test/mochitest/browser_console_filters.js:6
(Diff revision 1)
>  // Check that the Browser Console does not use the same filter prefs as the Web
>  // Console.

This description should be updated.

If the description of this test is accurate, is this a good spot to add Telemetry testing? I would prefer Nicolas to give some feedback here.

::: devtools/client/webconsole/test/mochitest/browser_console_filters.js:98
(Diff revision 1)
> +    ok(extra.inactive, expected.extra.inactive, "inactive is correct");
> +    ok(extra.active, expected.extra.active, "active is correct");

You should use is() to compare with expected values here.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Thanks Julian for thinking about mocha tests.
This patch does make ~10 mocha tests to fail, but the fix is pretty simple: 

diff --git a/devtools/client/webconsole/test/mocha-test-setup.js b/devtools/client/webconsole/test/mocha-test-setup.js
--- a/devtools/client/webconsole/test/mocha-test-setup.js
+++ b/devtools/client/webconsole/test/mocha-test-setup.js
@@ -75,7 +75,9 @@ requireHacker.global_hook("default", (pa
     case "devtools/client/webconsole/utils/context-menu":
       return "{}";
     case "devtools/client/shared/telemetry":
-      return `module.exports = function() {}`;
+      return `module.exports = function() {
+        this.recordEvent = () => {};
+      }`;

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990257 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

https://reviewboard.mozilla.org/r/255294/#review262184

::: commit-message-afdeb:1
(Diff revision 1)
> +Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry r?jdescottes

add ,nchevobbe as well

::: devtools/client/webconsole/actions/filters.js:30
(Diff revision 1)
> -function filterToggle(filter) {
> +function filterToggle(filter, sessionId) {
>    return (dispatch, getState, {prefsService}) => {
>      dispatch({
>        type: FILTER_TOGGLE,
>        filter,
>      });
>      const filterState = getAllFilters(getState());
> -    prefsService.setBoolPref(PREFS.FILTER[filter.toUpperCase()], filterState[filter]);
> +    const state = filterState[filter];
> +    const active = Object.keys(filterState)
> +                         .filter(key => key !== "text" ? filterState[key] : "");
> +    const inactive = Object.keys(filterState)
> +                           .filter(key => key !== "text" ? !filterState[key] : "");
> +
> +    telemetry.recordEvent("devtools.main", "filter_changed", "webconsole", null, {
> +      "clicked": filter,
> +      "active": active.join(","),
> +      "inactive": inactive.join(","),
> +      "session_id": sessionId
> +    });
> +    prefsService.setBoolPref(PREFS.FILTER[filter.toUpperCase()], state);

so, instead of cluttering the action here, I'd rather add a middleware for that (you can look at https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/devtools/client/webconsole/middleware/history-persistence.js#20 & https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/devtools/client/webconsole/store.js#26,80 to see an example of existing middleware)

I find it better to reason about side-effects of the application.

::: devtools/client/webconsole/components/FilterButton.js:33
(Diff revision 1)
> -      dispatch(actions.filterToggle(filterKey));
> +      const frame = event.target.ownerGlobal.frameElement;
> +      let sessionId = -1;
> +
> +      if (frame && frame.ownerGlobal.frameElement) {
> +        sessionId = frame.ownerGlobal.frameElement.getAttribute("session_id");
> +      }
> +
> +      dispatch(actions.filterToggle(filterKey, sessionId));

I agree with Julian here. Our components should be the dumbest possible :)

::: devtools/client/webconsole/test/mochitest/browser_console_filters.js:12
(Diff revision 1)
> +const OPTOUT = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
> +
> +// We only go though the filterClicked() code path once in this test but that is
> +// enough to test the telemetry event.
> +const DATA = [
> +  {
> +    timestamp: null,
> +    category: "devtools.main",
> +    method: "filter_changed",
> +    object: "webconsole",
> +    value: null,
> +    extra: {
> +      clicked: "error",
> +      active: "warn,log,info,debug",
> +      inactive: "error,css,net,netxhr"
> +    }
> +  }
> +];

I'd prefer having a specific dedicated telemetry test (browser_console_filters_telemetry)

Also, is this wanted to test specifically the browser console and not the webconsole ?

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8990257 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

https://reviewboard.mozilla.org/r/255294/#review262192

::: devtools/client/webconsole/actions/filters.js:39
(Diff revision 1)
> +                         .filter(key => key !== "text" ? filterState[key] : "");
> +    const inactive = Object.keys(filterState)
> +                           .filter(key => key !== "text" ? !filterState[key] : "");

Because text is not a filter.

::: devtools/client/webconsole/components/FilterButton.js:33
(Diff revision 1)
> -      dispatch(actions.filterToggle(filterKey));
> +      const frame = event.target.ownerGlobal.frameElement;
> +      let sessionId = -1;
> +
> +      if (frame && frame.ownerGlobal.frameElement) {
> +        sessionId = frame.ownerGlobal.frameElement.getAttribute("session_id");
> +      }
> +
> +      dispatch(actions.filterToggle(filterKey, sessionId));

That is fine. The point of this patch is to show you how it can be done.

I am going to leave this patch with you and you can change it to fit your architecture as you wish.

::: devtools/client/webconsole/test/mochitest/browser_console_filters.js:12
(Diff revision 1)
> +const OPTOUT = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
> +
> +// We only go though the filterClicked() code path once in this test but that is
> +// enough to test the telemetry event.
> +const DATA = [
> +  {
> +    timestamp: null,
> +    category: "devtools.main",
> +    method: "filter_changed",
> +    object: "webconsole",
> +    value: null,
> +    extra: {
> +      clicked: "error",
> +      active: "warn,log,info,debug",
> +      inactive: "error,css,net,netxhr"
> +    }
> +  }
> +];

As I said previously, this patch is an example of an implementation and I think you know enough to work on it yourself from here.

If you want a separate test you are more than welcome to create one.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8990257 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

https://reviewboard.mozilla.org/r/255294/#review262184

> I'd prefer having a specific dedicated telemetry test (browser_console_filters_telemetry)
> 
> Also, is this wanted to test specifically the browser console and not the webconsole ?

As I said before, this is just an example and you are free to implement it however you wish.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8990257 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255294/diff/1-2/

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Events.yaml:
devtools.main:
  filter_changed:
    objects: ["webconsole"]
    bug_numbers: [1463095]
    notification_emails: ["[e-mail]", "[e-mail]"]
    record_in_processes: ["main"]
    description: User has changed a filter in the web console.
    release_channel_collection: opt-out
    expiry_version: never
    extra_keys:
      clicked: "The clicked filter: error, warn, log, info, debug, css, netxhr or net"
      active: Comma separated list of active filters.
      inactive: Comma separated list of inactive filters.
      session_id: The toolbox session start time e.g. 13963.

Usage:
this.telemetry.recordEvent("devtools.main", "filter_changed", "webconsole", null, {
  clicked: "css",
  active: "error,warn,log,css",
  inactive: "info,debug,netxhr,net",
  session_id: toolbox.sessionId
});

Comment 11

[:Harald Kirschner :digitarald]

ni? Nicolas to prioritize this for 63.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

This introduces an event-telemetry middleware that we'll
be able to re-use for other events.
A test is added to make sure we do log those events as intended.
The telemetry mock for mocha test is modified to include
recordEvent so the test still run.

Review commit: https://reviewboard.mozilla.org/r/259306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259306/

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

TRY looks okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee89eb44281930f5a54023b1a250802e31b7166a

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8994775 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

https://reviewboard.mozilla.org/r/259306/#review266348

Looks really good, I guess the telemetry middleware is probably a pattern we should use in other panels. 
Some comments and questions, but nothing blocking.

::: devtools/client/webconsole/middleware/event-telemetry.js:9
(Diff revision 1)
> +
> +"use strict";
> +
> +const {
> +  FILTER_TEXT_SET,
> +  FILTER_TOGGLE,

Not directly related, but is FILTERS_CLEAR no longer used? I can't seem to find a code path that could lead to calling it?

::: devtools/client/webconsole/middleware/event-telemetry.js:26
(Diff revision 1)
> +    const res = next(action);
> +    if (!sessionId) {
> +      return res;
> +    }
> +
> +    const telemetry = new Telemetry();

Not sure how much we should care about performance in middleware code, but instanciating a new Telemetry instance triggers a dozen of Function.bind. 

Maybe move everything that is needed to log the event inside of the if(){} block? Maybe have a single Telemetry instance? We should probably move away from a class to something more static for this helper.

Not blocking on this, since I don't know how often this will be called.

::: devtools/client/webconsole/test/mochitest/browser_webconsole_telemetry_filters_changed.js:60
(Diff revision 1)
> +  const resetButton = await waitFor(() =>
> +    hud.ui.window.document.querySelector(".reset-filters-button"));
> +  const onResetButtonHidden = waitFor(() =>
> +    !hud.ui.window.document.querySelector(".reset-filters-button"));
> +  resetButton.click();
> +  await onResetButtonHidden;

Reading this I thought "surely this code must already be in some other mochitest checking the reset-filters button". But looks like this is the first mochitest testing the feature :)

::: devtools/client/webconsole/test/mochitest/browser_webconsole_telemetry_filters_changed.js:86
(Diff revision 1)
> +    ...expectedEvent,
> +    "session_id": event.session_id
> +  }), "The event has the expected data");
> +}
> +
> +function getFiltersChangedEventsExtra() {

Could we have a comment explaining what the function is doing and why event[5] has the data we are interested in.

::: devtools/client/webconsole/webconsole-output-wrapper.js:41
(Diff revision 1)
>    this.throttledDispatchPromise = null;
>  
>    this.telemetry = new Telemetry();
>  
> -  store = configureStore(this.hud);
> +  store = configureStore(this.hud, {
> +    sessionId: this.toolbox && this.toolbox.sessionId

Maybe a small comment to remind the reader that the BrowserConsole (I think?) is instanciated without a toolbox?

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the review Julian

> Not directly related, but is FILTERS_CLEAR no longer used? I can't seem to find a code path that could lead to calling it?

It's not use in the webconsole itself, but we do use it in some tests https://searchfox.org/mozilla-central/search?q=filtersClear()&case=false&regexp=false&path=

> Reading this I thought "surely this code must already be in some other mochitest checking the reset-filters button". But looks like this is the first mochitest testing the feature :)

Yes, and I don't like that too :) It's not intended to test the button per-se, but rather to ensure the button is ready to be clicked / the action was dispatched.

> Could we have a comment explaining what the function is doing and why event[5] has the data we are interested in.

Sure

> Maybe a small comment to remind the reader that the BrowserConsole (I think?) is instanciated without a toolbox?

Yes (and you're right, no toolbox for the browser console). Which makes me think, in other places Mike used -1 as the sessionId for browser console, so I think I'm going to do this as well.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8994775 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259306/diff/1-2/

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8994775 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

Re-asking for review since there was some changes in the patch (now passing the existing telemetry object instead of creating a new one)

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8994775 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

https://reviewboard.mozilla.org/r/259306/#review266578

Thanks Nicolas!

::: devtools/client/webconsole/store.js:82
(Diff revision 2)
>  
>    // Prepare middleware.
>    const middleware = applyMiddleware(
>      thunk.bind(null, {prefsService}),
>      historyPersistence,
> +    eventTelemetry.bind(null, options && options.telemetry, options && options.sessionId),

Is this check still necessary? Don't we always have telemetry and sessionId in options now?

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #18)
> Comment on attachment 8994775 [details]
> Bug 1463095 - Instrument inspection of filter changes in the Web Console
> with event telemetry; .
> 
> https://reviewboard.mozilla.org/r/259306/#review266578
> 
> Thanks Nicolas!
> 
> ::: devtools/client/webconsole/store.js:82
> (Diff revision 2)
> >  
> >    // Prepare middleware.
> >    const middleware = applyMiddleware(
> >      thunk.bind(null, {prefsService}),
> >      historyPersistence,
> > +    eventTelemetry.bind(null, options && options.telemetry, options && options.sessionId),
> 
> Is this check still necessary? Don't we always have telemetry and sessionId
> in options now?

Yes, we still want it. The store can be initialized from other places than webconsole-output-wrapper (e.g. mocha tests), and I'd rather make the check here than passing a stub from there. Does it make sense ?

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
> 
> Yes, we still want it. The store can be initialized from other places than
> webconsole-output-wrapper (e.g. mocha tests), and I'd rather make the check
> here than passing a stub from there. Does it make sense ?

I would say, if it's just mocha tests, it would make more sense to have them provide a mock here. Also we are not checking if "telemetry" is not null in the middleware, so I guess the middleware will throw in those cases?

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #20)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
> 
> I would say, if it's just mocha tests, it would make more sense to have them
> provide a mock here. Also we are not checking if "telemetry" is not null in
> the middleware, so I guess the middleware will throw in those cases?

I agree, i'll make the change.
Mocha weren't failing since we are guarding with sessionId in the middleware (but I can pass -1 from mocha where we configure the store).

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8994775 [details]
Bug 1463095 - Instrument inspection of filter changes in the Web Console with event telemetry; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259306/diff/2-3/

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

This should be good now (https://reviewboard.mozilla.org/r/259304/diff/2-3/) - I'll push in a bit with you don't have any objection.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa96db1c61bc
Instrument inspection of filter changes in the Web Console with event telemetry; r=jdescottes.

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fa96db1c61bc