1416024 - Record event for toolbox open

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Comment 1

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

Because event telemetry can have a bunch of properties sent and they can be from different codepaths I have set up a system where the events and properties can be sent separately.

To grasp how the API works you are best reading over the documentation patch.

Incidentally, because tests completely bypass the code where some of these pings are sent from we can't test it, which is annoying but I am expecting to be able to test all of the future event telemetry.

You can test locally using about:telemetry.

We don't currently include the browser toolbox either but may change that in the future.

@hkirschner: The browser toolbox is not currently included... would you like it to be or would you prefer that it is added as a separate event?

Comment 2

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

Attachment: Bug 1416024 - Patch 1: Record event for toolbox open/close

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

Comment 3

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

Attachment: Bug 1416024 - Patch 2: Add Event telemetry documentation

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

Comment 4

[:Harald Kirschner :digitarald]

@miker, to clarify, is the browser toolbox also not included for existing telemetry probes? What about remote debugging sessions in WebIDE?

Seems like the requirements didn't cover that, so we should push that to follow up work. What would be the extra effort?

Comment 5

[Yulia Startsev [:yulia]]

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

https://reviewboard.mozilla.org/r/232502/#review238732

::: devtools/client/framework/devtools.js:488
(Diff revision 1)
>        }
>        this._firstShowToolbox = false;
>      }
> +
> +    this._telemetry.addEventProperty(
> +      "devtools.main", "open", "tools", null, "width", toolbox.win.outerWidth);

should "value" be null?

::: devtools/client/shared/telemetry.js:422
(Diff revision 1)
> +   *
> +   * @param {String} category
> +   *        The telemetry event category e.g. "devtools.main"
> +   * @param {String} method
> +   *        The telemetry event method e.g. "open"
> +   * @param {String} object

maybe we can use a name other than "object" here, as its confusing to have a string named object. The description is also a little confusing because i would expect it to be an object, except for the param being a string. maybe "name"? or similar? and updating the description?

::: devtools/client/shared/telemetry.js:422
(Diff revision 1)
> +   *
> +   * @param {String} category
> +   *        The telemetry event category e.g. "devtools.main"
> +   * @param {String} method
> +   *        The telemetry event method e.g. "open"
> +   * @param {String} object

this nameing was a bit confusing so i took a look in the telemetry docs. maybe we can borrow their description of this field? with a slight modification, saying that its the object _name_ 

"This is the name of the object the event occurred on. e.g. "tools" or "setting""

::: devtools/client/shared/telemetry.js:441
(Diff revision 1)
> +   *        [
> +   *          "host",
> +   *          "width"
> +   *        ]
> +   */
> +  preparePendingEvent(category, method, object, value, extra = {}, expected = []) {

I am wondering about the "extra" field here. I would have expected it to be used to add fields that could be added early, but this is not how it is done in the file 

    devtools/client/framework/toolbox.js
  
starting at line 731, even though the line is followed by an "addEventProperty" function which adds an event that could be here? I am wondering if this is intentional? Maybe this needs to be commented?

Otherwise, I think removing it has a lot of benefits.

We will be able to simplify this by not needing the error and also not modify the "extra" argument (which feels strange in any case)

Also, maybe we don't have to use the same function signiture as the recordEvent? if we move value to the end, it can be defaulted to null, and then we do not have to add it to the argument list unless it is something different? Anything that can be defaulted should be at the end. 

Last item: I expect that "expected" should never be an empty array? we should probably throw if expected is empty, because then there is no reason to create a pending event

::: devtools/client/shared/telemetry.js:450
(Diff revision 1)
> +      throw new Error(`preparePendingEvent() was called with the signature ` +
> +                      `"${sig}" but all of the expected events are already ` +
> +                      `present. Please call recordEvent() directly instead.`);
> +    }
> +
> +    extra.expected = new Set(expected);

this modification of the argument is really confusing. we are adding and deleting things from extra when we could have these two things separate in the map. Why not do
`PENDING_EVENTS.set(sig, { extra, expected }` ?

Also, we cannot rule out the possiblity that extra will have a field called expected, since expected cannot be made private.

::: devtools/client/shared/telemetry.js:482
(Diff revision 1)
> +   *        The pending property name
> +   * @param {String} pendingPropValue
> +   *        The pending property value
> +   */
> +  addEventProperty(category, method, object, value, pendingPropName, pendingPropValue) {
> +    let sig = `${category},${method},${object},${value}`;

nit: im seeing let in a few places where we could use const, maybe we can replace those so its clearer that these variables should not change

::: devtools/client/shared/telemetry.js:484
(Diff revision 1)
> +   *        The pending property value
> +   */
> +  addEventProperty(category, method, object, value, pendingPropName, pendingPropValue) {
> +    let sig = `${category},${method},${object},${value}`;
> +
> +    if (!PENDING_EVENTS.has(sig)) {

I think this could use a comment! It took me a minute to realize that this was adding the event property to a list, in case the PENDING_EVENTS hadn't been initialized yet

::: devtools/client/shared/telemetry.js:491
(Diff revision 1)
> +        [pendingPropName]: pendingPropValue
> +      });
> +      return;
> +    }
> +
> +    let extra = PENDING_EVENTS.get(sig);

as per the recommendation I made above, this could become `let { extra, expected } = PENDING_EVENTS.get(sig);`

And then there won't be the thing about the funny delete!

Comment 6

[Yulia Startsev [:yulia]]

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

https://reviewboard.mozilla.org/r/232504/#review238738

::: devtools/docs/frontend/telemetry.md:219
(Diff revision 1)
> +
> +### 4. Using Events.yaml probes in DevTools code
> +
> +Once the probe has been declared in the `Events.yaml` file, you'll need to actually use it in our code.
> +
> +It is crucial to understand that event telemetry contains compound keys that are made up from `category`, `method`, `object` and `value`. This key points to an "extra" object that contains further information about the event (we will give examples later in this section).

what does "compound keys" mean? maybe it would be clearer to say "have a string identifier which is constructed from the category, method, object name on which the event occured, and value?

Comment 7

[:Harald Kirschner :digitarald]

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

https://reviewboard.mozilla.org/r/232504/#review238922

::: devtools/docs/frontend/telemetry.md:12
(Diff revision 1)
>  The process to add metrics to a tool roughly consists in:
>  
>  1. Adding the probe to Firefox
>  2. Using Histograms.json probes in DevTools code
>  3. Using Scalars.yaml probes in DevTools code
> -4. Getting approval from the data team
> +4. Using Events.yaml probes in DevTools code

Important to note is that we are not just collecting Event Telemetry but that the events are intended to be analyzed in Amplitude; so they have to follow consistent formatting and some extra rigor & planning.

::: devtools/docs/frontend/telemetry.md:385
(Diff revision 1)
>  
>  This is required before the changes make their way into `mozilla-central`.
>  
>  To get approval, attach your patch to the bug in Bugzilla, and set two flags:
>  
> -* a `feedback?` flag for bsmedberg (or someone else from the data team)
> +- a `feedback?` flag for bsmedberg (or someone else from the data team)

bsmedberg left. More importantly the process changed, as documented here: https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Data_Collection with data stewards and a link to the request form.

Comment 8

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

https://reviewboard.mozilla.org/r/232502/#review238744

::: devtools/client/framework/devtools.js:488
(Diff revision 1)
>        }
>        this._firstShowToolbox = false;
>      }
> +
> +    this._telemetry.addEventProperty(
> +      "devtools.main", "open", "tools", null, "width", toolbox.win.outerWidth);

Yes, if you don't want to use a value you use null.

::: devtools/client/shared/telemetry.js:422
(Diff revision 1)
> +   *
> +   * @param {String} category
> +   *        The telemetry event category e.g. "devtools.main"
> +   * @param {String} method
> +   *        The telemetry event method e.g. "open"
> +   * @param {String} object

These are the property names used by the official API so they should stay the same:

```
Services.telemetry.recordEvent(category, method, object, value, extra);
```

I completely agree that they are confusing though so I have borrowed the descriptions from that API.

::: devtools/client/shared/telemetry.js:441
(Diff revision 1)
> +   *        [
> +   *          "host",
> +   *          "width"
> +   *        ]
> +   */
> +  preparePendingEvent(category, method, object, value, extra = {}, expected = []) {

I like the idea of removing extra to simplify things a little.

I don't want to default value to null because it will normally have a value and we should preserve the signature (category, method, object, value).

We now throw on an empty expected Array.

::: devtools/client/shared/telemetry.js:450
(Diff revision 1)
> +      throw new Error(`preparePendingEvent() was called with the signature ` +
> +                      `"${sig}" but all of the expected events are already ` +
> +                      `present. Please call recordEvent() directly instead.`);
> +    }
> +
> +    extra.expected = new Set(expected);

Agreed, now using:

```
PENDING_EVENTS.set(sig, {
  extra: {},
  expected: new Set(expected)
});
```

::: devtools/client/shared/telemetry.js:482
(Diff revision 1)
> +   *        The pending property name
> +   * @param {String} pendingPropValue
> +   *        The pending property value
> +   */
> +  addEventProperty(category, method, object, value, pendingPropName, pendingPropValue) {
> +    let sig = `${category},${method},${object},${value}`;

The new methods now only use let where we can't use const.

::: devtools/client/shared/telemetry.js:484
(Diff revision 1)
> +   *        The pending property value
> +   */
> +  addEventProperty(category, method, object, value, pendingPropName, pendingPropValue) {
> +    let sig = `${category},${method},${object},${value}`;
> +
> +    if (!PENDING_EVENTS.has(sig)) {

Added

::: devtools/client/shared/telemetry.js:491
(Diff revision 1)
> +        [pendingPropName]: pendingPropValue
> +      });
> +      return;
> +    }
> +
> +    let extra = PENDING_EVENTS.get(sig);

Already fixed as part of your earlier comment.

Comment 9

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

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

Comment 10

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

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

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

Comment 11

[Yulia Startsev [:yulia]]

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

https://reviewboard.mozilla.org/r/232502/#review239156

Looks good!

Comment 12

[Yulia Startsev [:yulia]]

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

https://reviewboard.mozilla.org/r/232504/#review238740

::: devtools/docs/frontend/telemetry.md:257
(Diff revision 1)
> +
> +// If your "extra" properties are in different code paths you will need to
> +// create a "pending event." These events contain a list of expected properties
> +// that can be populated before or after creating the pending event.
> +
> +// Use the category, method, object, value combinations above to add a

though i know that object is what the telemetry team uses for this field, i find it super confusing! not an issue just a note :)

Comment 13

[Yulia Startsev [:yulia]]

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

https://reviewboard.mozilla.org/r/232504/#review239158

Looks good!

Comment 14

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

Attachment: data-review.txt

Comment 15

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

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

Comment 16

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

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

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

Comment 17

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

https://reviewboard.mozilla.org/r/232500/#review239358

We were removing items from the expected properties list each time one was received. This mean that whenever a property was sent more than once we threw an exception because the property was no longer expected.

We now no longer remove anything from the expected properties list but wait until the "extra" list is the same length as the expected properties list before sending the event.

Comment 18

[François Marier [:francois]]

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

> 5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox [data collection categories](https://wiki.mozilla.org/Firefox/Data_Collection) on the Mozilla wiki.
> 
> Measurement Description: devtools.main.open

Could you please expand on the description to include the data that is actually collected in the ping?

Looking at https://reviewboard.mozilla.org/r/232502/diff/3#index_header, I see a couple of things:

- entrypoint: fixed list of strings
- panel: arbitrary string? can user strings end up in here?
- host: fixed list of strings
- splitconsole: true/false
- width: toolbox width in pixels, presumably also the width of the browser window

With respect to the width, do you need an exact amount or could you round it up to the nearest 50px or 100px? It would help reduce the likelihood that this piece of data could be used to identify a user.

> 9. Please provide a general description of how you will analyze this data.
> Using Amplitude, see https://docs.google.com/spreadsheets/d/1PvLMiSQeq-XMPhgUr1GpVGCtg64XwGhOMQ8hZSYR07Q/edit#gid=879531055

This document is not publicly accessible. If it can be made public, then that's great, otherwise we should remove it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c25684c9948
Patch 1: Record event for toolbox open/close r=yulia
https://hg.mozilla.org/integration/autoland/rev/ca1744840f1a
Patch 2: Add Event telemetry documentation r=yulia

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76dbe7dfdea2
Backed out 2 changesets for failures on browser/base/content/test/performance/browser_startup.js a=backout on a CLOSED TREE

Comment 21

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

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

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

Comment 22

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

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

Comment 23

[:Harald Kirschner :digitarald]

> With respect to the width, do you need an exact amount or could you round it up to the nearest 50px or 100px? It would help reduce the likelihood that this piece of data could be used to identify a user.

OK. Steps of 50px should be enough to answer our questions about responsive design.

> - panel: arbitrary string? can user strings end up in here?

Extension IDs can end up here, which is intentional as they are important to know about as part of user flow.

We probably have some issue though as I checked other probes [1] that collect extension ids for panels and they collect considerable noise (which I file as a bug with a recommendation as soon as we have figured out a way forward here).

:mratcliffe, can we transform extension panels by cutting off the cruft in the end?

Examples:
_react-devtools-20815-0-devtools-panel -> _react-devtools
_13d75c08-fed6-4ff6-a356-d79509b17e44_-10124-0-devtools-panel -> 5caff8cc-3d2e-4110-a88a-003cc85b3858

> This document is not publicly accessible. If it can be made public, then that's great, otherwise, we should remove it.

This can be removed, the sheet does not talk about the analysis anyways. The analysis will happen in Amplitude and STMO.

:francois, does this answer your questions?

[1]: https://mzl.la/2Iwh1fM see keys dropdown.

Comment 24

[François Marier [:francois]]

(In reply to :Harald Kirschner :digitarald from comment #23)
> > - panel: arbitrary string? can user strings end up in here?
> 
> Extension IDs can end up here, which is intentional as they are important to
> know about as part of user flow.

Cool, that's fine then. Category 1.

> Examples:
> _react-devtools-20815-0-devtools-panel -> _react-devtools
> _13d75c08-fed6-4ff6-a356-d79509b17e44_-10124-0-devtools-panel ->
> 5caff8cc-3d2e-4110-a88a-003cc85b3858

I assume these GUIDs are unique per Web Extension, not per user, right?

> :francois, does this answer your questions?

Yes, thanks.

If you can update data-review.txt with the extra details in Question 5 and remove the link from Question 9, I'll be happy to review again.

Comment 25

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

(In reply to François Marier [:francois] from comment #24)
> (In reply to :Harald Kirschner :digitarald from comment #23)
> > Examples:
> > _react-devtools-20815-0-devtools-panel -> _react-devtools
> > _13d75c08-fed6-4ff6-a356-d79509b17e44_-10124-0-devtools-panel ->
> > 5caff8cc-3d2e-4110-a88a-003cc85b3858
> 
> I assume these GUIDs are unique per Web Extension, not per user, right?
> 

Yes, these are per extension.

Comment 26

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

Attachment: data-review.txt

Comment 27

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232502/diff/4-5/

Comment 28

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232502/diff/5-6/

Comment 29

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

Comment on attachment 8963589 [details]
Bug 1416024 - Patch 2: Add Event telemetry documentation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232504/diff/4-5/

Comment 30

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

Comment on attachment 8963588 [details]
Bug 1416024 - Patch 1: Record event for toolbox open/close

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232502/diff/6-7/

Comment 31

[François Marier [:francois]]

Comment on attachment 8965615 [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, in 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.

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 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da48cddf7c34
Patch 2: Add Event telemetry documentation r=yulia
https://hg.mozilla.org/integration/autoland/rev/662199c2ab8d
Patch 1: Record event for toolbox open/close r=yulia

Comment 33

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/da48cddf7c34
https://hg.mozilla.org/mozilla-central/rev/662199c2ab8d

Comment 34

[:Harald Kirschner :digitarald]

Moving to M1 for splitting off M2 work.