1465694 - Add tab event telemetry for Savant Shield study

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Bianca Danforth [:bdanforth]]

This is one of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).

This bug will register and record (for the duration of the study only):
* When the user opens a tab.
* When the user closes a tab.
* When the user selects a tab.
* When the user moves a tab.

Comment 1

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465694 - Add tab event telemetry for Savant Shield study;

These probes will register and record (for the duration of the study only):
* When a tab opens.
* When a tab closes.
* When a tab is selected.
* When a tab moves.

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

Comment 2

[Bianca Danforth [:bdanforth]]

:pdehaan, could you provide QA review on this patch? Instructions for obtaining the test build are here: https://github.com/biancadanforth/gecko-dev/issues/5

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

https://reviewboard.mozilla.org/r/249544/#review255798

::: browser/base/content/tabbrowser.js:1043
(Diff revision 1)
>          detail: {
>            previousTab: oldTab
>          }
>        });
>        newTab.dispatchEvent(event);
> +      Services.telemetry.recordEvent("savant", "tab", "select", null, { subcategory: "frame" });

Forgive my ignorance, what does subcategory: "frame" mean here?

::: browser/base/content/tabbrowser.js:1043
(Diff revision 1)
>          detail: {
>            previousTab: oldTab
>          }
>        });
>        newTab.dispatchEvent(event);
> +      Services.telemetry.recordEvent("savant", "tab", "select", null, { subcategory: "frame" });

So recordEvent's signature is:

void recordEvent(aCategory, aMethod, aObject, [optional] aValue, [optional] extra);

"tab" as the method and "select" as the object seems a bit backwards, but I think you probably did the right thing here and this signature is just weird...

::: browser/base/content/tabbrowser.js:3518
(Diff revision 1)
>      this.tabContainer._setPositionalAttributes();
>  
>      var evt = document.createEvent("UIEvents");
>      evt.initUIEvent("TabMove", true, false, window, oldPosition);
>      aTab.dispatchEvent(evt);
> +    Services.telemetry.recordEvent("savant", "tab", "move", null, { subcategory: "frame" });

I don't really see how this is useful information for onboarding / retention, the other probes seem way more interesting. I'd err on the side of collecting less data unless there's actually a good reason to collect it.

::: browser/base/content/tabbrowser.js:3518
(Diff revision 1)
>      this.tabContainer._setPositionalAttributes();
>  
>      var evt = document.createEvent("UIEvents");
>      evt.initUIEvent("TabMove", true, false, window, oldPosition);
>      aTab.dispatchEvent(evt);
> +    Services.telemetry.recordEvent("savant", "tab", "move", null, { subcategory: "frame" });

I don't really see how this is useful information for onboarding / retention, the other probes seem way more interesting. I'd err on the side of collecting less data unless there's actually a good reason to collect it.

Comment 4

[Bianca Danforth [:bdanforth]]

I neglected to mention that for the event telemetry to show up in `about:telemetry#events-tab`, the study pref `shield.savant.enabled` needs to be set to `true`. Normandy will take care of turning this pref ON and OFF when the study is officially launched.

Comment 5

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

https://reviewboard.mozilla.org/r/249544/#review256066

::: browser/base/content/tabbrowser.js:1043
(Diff revision 1)
>          detail: {
>            previousTab: oldTab
>          }
>        });
>        newTab.dispatchEvent(event);
> +      Services.telemetry.recordEvent("savant", "tab", "select", null, { subcategory: "frame" });

Good question. This field was outlined in a probe spec[1] that I am operating on created by the study's analyst, :shong. I'm not sure exactly what question(s) he's hoping to answer here, but I can say in general that this tab probe is one of a number of other probes which can have varied subcategories like "feature", "navigation", etc. This subcategory is to help differentiate this event from other kinds of events we are interested in.
[1]:https://docs.google.com/spreadsheets/d/192unlFYdQ_nc5UVaG79YhY2bp6bV7XZm1G4BW-etOLE/edit#gid=1257575649

Yes, you are right. Currently, we are not following the intended taxonomy for event telemetry. It was recommended to me by :chutten for simplicity to have one "savant" category of event, which caused the values of the remaining arguments to need to shift around. This simplifies development/testing, because event telemetry is disabled by default and is enabled on a per-category basis, so we have one category instead of five categories. This also helps isolate these probes from being enabled/disabled by others and keeps all study-related probes under one logical top level key. If we were to make these probes permanent, we would need to restructure them to more closely follow the taxonomy.

::: browser/base/content/tabbrowser.js:3518
(Diff revision 1)
>      this.tabContainer._setPositionalAttributes();
>  
>      var evt = document.createEvent("UIEvents");
>      evt.initUIEvent("TabMove", true, false, window, oldPosition);
>      aTab.dispatchEvent(evt);
> +    Services.telemetry.recordEvent("savant", "tab", "move", null, { subcategory: "frame" });

This is one of 13 probes (see previous comment for spec sheet). Taken together, I think the tab will be helpful as being one way of further subdividing a session into a sequences of events.
The 13 probes were actually prioritized as "must have" from the stakeholders, cmore and rtestard, narrowed down from a list of 60 or 70 probes.

Comment 6

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

https://reviewboard.mozilla.org/r/249544/#review255798

> So recordEvent's signature is:
> 
> void recordEvent(aCategory, aMethod, aObject, [optional] aValue, [optional] extra);
> 
> "tab" as the method and "select" as the object seems a bit backwards, but I think you probably did the right thing here and this signature is just weird...

Edit: I tried to respond to this inline and it somehow got dissociated, so going to paste my reply here; sorry about that:

Good question. This field was outlined in a probe spec1 that I am operating on created by the study's analyst, :shong. I'm not sure exactly what question(s) he's hoping to answer here, but I can say in general that this tab probe is one of a number of other probes which can have varied subcategories like "feature", "navigation", etc. This subcategory is to help differentiate this event from other kinds of events we are interested in.

Yes, you are right. Currently, we are not following the intended taxonomy for event telemetry. It was recommended to me by :chutten for simplicity to have one "savant" category of event, which caused the values of the remaining arguments to need to shift around. This simplifies development/testing, because event telemetry is disabled by default and is enabled on a per-category basis, so we have one category instead of five categories. This also helps isolate these probes from being enabled/disabled by others and keeps all study-related probes under one logical top level key. If we were to make these probes permanent, we would need to restructure them to more closely follow the taxonomy.

> I don't really see how this is useful information for onboarding / retention, the other probes seem way more interesting. I'd err on the side of collecting less data unless there's actually a good reason to collect it.

Edit: I tried to respond to this inline and it somehow got dissociated, so going to paste my reply here; sorry about that:

This is one of 13 probes (see previous comment for spec sheet). Taken together, I think the tab will be helpful as being one way of further subdividing a session into a sequences of events.
The 13 probes were actually prioritized as "must have" from the stakeholders, cmore and rtestard, narrowed down from a list of 60 or 70 probes.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Bianca Danforth [:bdanforth] from comment #6)
> This is one of 13 probes (see previous comment for spec sheet). Taken
> together, I think the tab will be helpful as being one way of further
> subdividing a session into a sequences of events.
> The 13 probes were actually prioritized as "must have" from the
> stakeholders, cmore and rtestard, narrowed down from a list of 60 or 70
> probes.

I think they might be misunderstanding what a tab move is? The document you pointed to has "Client opens, closes, or switches a tab" as the event description; tab move has nothing to do with these, and will probably add noise to the data set.

Comment 8

[Bianca Danforth [:bdanforth]]

(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Bianca Danforth [:bdanforth] from comment #6)
> > This is one of 13 probes (see previous comment for spec sheet). Taken
> > together, I think the tab will be helpful as being one way of further
> > subdividing a session into a sequences of events.
> > The 13 probes were actually prioritized as "must have" from the
> > stakeholders, cmore and rtestard, narrowed down from a list of 60 or 70
> > probes.
> 
> I think they might be misunderstanding what a tab move is? The document you
> pointed to has "Client opens, closes, or switches a tab" as the event
> description; tab move has nothing to do with these, and will probably add
> noise to the data set.

You're right that there is a discrepancy between the qualitative description and the list of events in the `objects` field in the spec. After talking with :shong, the study's analyst, the "move" event doesn't seem to be as useful as the other ones, so I will take it out.

Comment 9

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

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

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

https://reviewboard.mozilla.org/r/249544/#review256538

Thanks!

Comment 11

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

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

Comment 12

[Peter deHaan [:pdehaan]]

R+ from me.

Got the following (deduped) events. Looks like the search/urlbar is unrelated.

| category | method     | object | Value  | extra
|----------|------------|--------|--------|-------
| savant   | search     | urlbar | enter  | {"subcategory": "navigation", "engine": "google"}
| savant   | tab        | close  | null   | {"subcategory": "frame"}
| savant   | tab        | open   | null   | {"subcategory": "frame"}
| savant   | tab        | select | null   | {"subcategory": "frame"}


I never got any "When the user moves a tab." flow, but from the looks of the code and comments above, that was yanked.

Comment 13

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a278eebbe5d
Add tab event telemetry for Savant Shield study; r=dao

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8a278eebbe5d

Comment 16

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]: There is no regression. The tracking bug for this study is 1457226.
[User impact if declined]: None. This patch is part of a priority Shield study.
[Is this code covered by automated tests?]: There are no study-specific tests, but all code has been run on the try server for regression testing and has been approved by both a Firefox peer and QA (pdehaan).
[Has the fix been verified in Nightly?]: The study’s functionality has been verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This request applies to all patches in this bug. In general, here is a comprehensive list of bugs whose patches need to be uplifted (in this order) to complete the study: 1462725, 1465698, 1465707, 1465703, 1465704, 1465697, 1465685, 1465694
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is an observational pref-flip study with no branches and no UI. It adds some event telemetry probes in various places and a study module to enable/disable collection. Each patch has been QA tested and R+ by pdehaan.
[String changes made/needed]: None.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983716 [details]
Bug 1465694 - Add tab event telemetry for Savant Shield study;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13af68f9e6d0