Add readermode event telemetry for Savant Shield study

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: bdanforth, Assigned: bdanforth)

Tracking

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed, firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
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 turns on Reader Mode.
* When the user turns off Reader Mode.
(Assignee)

Updated

10 months ago
Blocks: 1457226
(Assignee)

Updated

10 months ago
Depends on: 1462725
(Assignee)

Updated

10 months ago
Assignee: nobody → bdanforth
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

10 months ago
: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
Flags: needinfo?(pdehaan)
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255936

::: browser/base/content/tab-content.js:118
(Diff revision 1)
>      switch (message.name) {
>        case "Reader:ToggleReaderMode":
>          if (!this.isAboutReader) {
>            this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
>            ReaderMode.enterReaderMode(docShell, content);
> +          Services.telemetry.recordEvent("savant", "readermode", "on", null,

I downloaded the build that you had built on tryserver[1] and was unable to get any events to show up in about:telemetry on the Events page.

Are you missing a call to setEventRecordingEnabled[2]? The comment next to this function says "Event telemetry is disabled by default. Use this method to enable it for a particular category."

Or did I test this wrong?

For comparison, I do see events logged for "activity_stream", and that category is enabled and disabled using the above function.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f776980a2fac69fed347e73e58e237340adfc2eb
[2] https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/devtools/client/shared/telemetry.js#323
Attachment #8983722 - Flags: review?(jaws) → review-
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255940

::: toolkit/components/telemetry/Events.yaml:135
(Diff revision 1)
> +    notification_emails:
> +      - "bdanforth@mozilla.com"
> +      - "shong@mozilla.com"
> +    expiry_version: "65"
> +    extra_keys:
> +      subcategory: The broad event category for this probe. E.g. navigation

The description of subcategory here looks very generic. Can you describe which subcategories can be expected? Since you're using "feature" in both cases, is this actually necessary?
(Assignee)

Comment 5

10 months ago
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. Note that the readermode probes will show up in the content process, rather than the parent process (there's a dropdown menu in `about:telemetry#events-tab` in the top right corner).
(Assignee)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255958

::: browser/base/content/tab-content.js:118
(Diff revision 1)
>      switch (message.name) {
>        case "Reader:ToggleReaderMode":
>          if (!this.isAboutReader) {
>            this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
>            ReaderMode.enterReaderMode(docShell, content);
> +          Services.telemetry.recordEvent("savant", "readermode", "on", null,

Sorry about that jaws, I just commented[1] in the bug to clear that up.

[1]:https://bugzilla.mozilla.org/show_bug.cgi?id=1465698#c5

::: toolkit/components/telemetry/Events.yaml:135
(Diff revision 1)
> +    notification_emails:
> +      - "bdanforth@mozilla.com"
> +      - "shong@mozilla.com"
> +    expiry_version: "65"
> +    extra_keys:
> +      subcategory: The broad event category for this probe. E.g. navigation

I am basing these values on a spec[1] that was given to me by the stakeholders and analyst for this study.

This probe is one of about 13, so if they are all being recorded, I imagine it will help the analyst (:shong) on the back-end.

[1]:https://docs.google.com/spreadsheets/d/192unlFYdQ_nc5UVaG79YhY2bp6bV7XZm1G4BW-etOLE/edit#gid=1257575649
If I exit Reader Mode using the button from within Reader Mode (the "Close Reader View" button), then the "off" doesn't get recorded.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

10 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> If I exit Reader Mode using the button from within Reader Mode (the "Close
> Reader View" button), then the "off" doesn't get recorded.

:jaws, the latest patch (rev 2) should fix that issue; thank you for pointing it out. I've moved the probe to ReaderMode.jsm, so the event should show up in the parent process now in `about:telemetry#events-tab`.
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255984

::: toolkit/components/telemetry/Events.yaml:126
(Diff revision 2)
>      extra_keys:
>        subcategory: The broad event category for this probe. E.g. navigation
> +  readermode:
> +    objects: ["on", "off"]
> +    release_channel_collection: opt-out
> +    record_in_processes: ["content"]

I believe this should be "main" now, right?
Attachment #8983722 - Flags: review?(jaws) → review-
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255998

r=me with the commit message saying that the events will appear in the content section.
Attachment #8983722 - Flags: review- → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249556/#review255984

> I believe this should be "main" now, right?

Hmm... actually it seems to be a "content" process event afterall. I left it as "content" in Events.yaml and updated the commit message to say it's a content process in rev 3.
R+ from QA. I can see the readermode methods toggle on/off in about:telemetry#events-tab (content process) when testing on apple.com and theverge.com, and both events stopped triggering after disabling savant in about:config.
Flags: needinfo?(pdehaan)

Comment 15

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f53778c7350f
Add readermode event probes for Savant Shield study; r=jaws

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f53778c7350f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(Assignee)

Comment 17

10 months ago
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

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.
Attachment #8983722 - Flags: approval-mozilla-beta?
Comment on attachment 8983722 [details]
Bug 1465698 - Add readermode event probes for Savant Shield study;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.
Attachment #8983722 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5a59ca5aacd6
status-firefox61: --- → fixed
You need to log in before you can comment on or make changes to this bug.