Closed Bug 1465698 Opened 6 years ago Closed 6 years ago

Add readermode event telemetry for Savant Shield study

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: bdanforth, Assigned: bdanforth)

References

Details

Attachments

(1 file)

Bug 1465698 - Add readermode event probes for Savant Shield study;
59 bytes, text/x-review-board-request
jaws
: review+
RyanVM
: approval-mozilla-beta+
Details 
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.
Blocks: 1457226
Depends on: 1462725
Assignee: nobody → bdanforth
Status: NEW → ASSIGNED
Priority: -- → P1
: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?
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).
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.
(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 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)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f53778c7350f
Add readermode event probes for Savant Shield study; r=jaws
https://hg.mozilla.org/mozilla-central/rev/f53778c7350f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: