1219240 - Telemetry probe to measure % of pages that show reader view button

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Bug 1219240 - Telemetry probe to measure % of pages that show reader view button. r=Gijs

Comment 2

[:Margaret Leibovic]

Gijs, this is something our product manager, Barbara, requested for mobile, but I figured it would be useful for desktop as well. What do you think? I'm happy to make this mobile-only if you don't think anyone will look at this for desktop.

Comment 3

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #2)
> Gijs, this is something our product manager, Barbara, requested for mobile,
> but I figured it would be useful for desktop as well. What do you think? I'm
> happy to make this mobile-only if you don't think anyone will look at this
> for desktop.

We'll need a privacy review for this.

For that, and because I'm not really sure: what will this number be used for? I'm struggling to understand what just that number will tell us - it won't say anything about the correctness of showing that icon, I think, and we don't seem to have data on how often people enter reader mode either (to measure engagement with the icon)...

Comment 4

[:Gijs (he/him)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

https://reviewboard.mozilla.org/r/23601/#review21129

Code-wise, this looks fine.

Comment 5

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> > Gijs, this is something our product manager, Barbara, requested for mobile,
> > but I figured it would be useful for desktop as well. What do you think? I'm
> > happy to make this mobile-only if you don't think anyone will look at this
> > for desktop.
> 
> We'll need a privacy review for this.

Yes, I'll flag it once we get the details sorted out here...

> For that, and because I'm not really sure: what will this number be used
> for? I'm struggling to understand what just that number will tell us - it
> won't say anything about the correctness of showing that icon, I think, and
> we don't seem to have data on how often people enter reader mode either (to
> measure engagement with the icon)...

We do have UI telemetry for this on mobile, so we're looking for the other piece of the picture. But maybe we should change this probe to instead be an enum of "button not shown", "button shown", "button shown and clicked".

Barbara, what would you be looking for here?

Comment 6

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #5)
> (In reply to :Gijs Kruitbosch from comment #3)
> > For that, and because I'm not really sure: what will this number be used
> > for? I'm struggling to understand what just that number will tell us - it
> > won't say anything about the correctness of showing that icon, I think, and
> > we don't seem to have data on how often people enter reader mode either (to
> > measure engagement with the icon)...
> 
> We do have UI telemetry for this on mobile

Ah! I looked through histograms.json, but I forgot about UITelemetry. That makes somewhat more sense, though as you say a single histogram is probably easier to analyze.

Comment 7

[Barbara Bermes [:barbara] - NI please!]

Primarily, I want to understand if people like the reader view.
how many people use the reader view icon when it's shown.

For example, if only 2% of the pages loaded in Fennec show the icon and 80% of people enter reader mode, it would tell us that this feature is something people like to use and we should consider re-evaluating when/how the reader icon shows up.

I hope that makes sense.

Please let me know if there is anything else you'd like to know.

From a feature engagement perspective, the READER_MODE_PARSE_RESULT doesn't really provide much information.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle

Comment 9

[:Margaret Leibovic]

This ended up being pretty specific to the UI, so unless there's a request for desktop data, I'm going to limit this to mobile for now.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Asking vladan to review the new histogram.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

https://reviewboard.mozilla.org/r/23601/#review21203

::: mobile/android/chrome/content/Reader.js:259
(Diff revision 2)
> +      this._buttonHistogram.add(this._buttonHistogramValues.HIDDEN);

The pageaction is shown here too. It's the special reader pageaction when the reader view is active. This should probably be SHOWN or maybe nothing if we don't want to count showing the pageaction when you're in the reader view.

You want to add an | else | block on line 269 and add HIDDEN there.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle

Comment 13

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 8680246 [details]
> MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction
> with reader view button. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/23601/#review21203
> 
> ::: mobile/android/chrome/content/Reader.js:259
> (Diff revision 2)
> > +      this._buttonHistogram.add(this._buttonHistogramValues.HIDDEN);
> 
> The pageaction is shown here too. It's the special reader pageaction when
> the reader view is active. This should probably be SHOWN or maybe nothing if
> we don't want to count showing the pageaction when you're in the reader view.
> 
> You want to add an | else | block on line 269 and add HIDDEN there.

Oops, that was a dumb mistake. Good catch.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Stupid mozreview...

Comment 15

[Vladan Djeric (:vladan)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

https://reviewboard.mozilla.org/r/23601/#review21333

::: toolkit/components/telemetry/Histograms.json:9045
(Diff revision 3)
> +    "alert_emails": "[e-mail]",

this field should be a 1-element array. does this Histogram.json get parsed properly? it shouldn't

::: toolkit/components/telemetry/Histograms.json:9086
(Diff revision 3)
> +    "n_values": 5,

make n_values=10 so it has room to grow. you won't be able to change the bucketing after you land the histogram

Comment 16

[:Margaret Leibovic]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #15)
> Comment on attachment 8680246 [details]
> MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction
> with reader view button. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/23601/#review21333
> 
> ::: toolkit/components/telemetry/Histograms.json:9045
> (Diff revision 3)
> > +    "alert_emails": "[e-mail]",
> 
> this field should be a 1-element array. does this Histogram.json get parsed
> properly? it shouldn't

Thanks, I'll fix that.

This patch did build and run... although I do see an error when I try loading about:telemetry. However, I still see this when running with a fixed build. Should I file a bug about these two issues (the histogram problem not causing an error, and about:telemetry running into a JS error)?

TypeError: date is undefined
this.TelemetryUtils.truncateToDays()
 TelemetryUtils.jsm:44
getMetadata()
 TelemetrySession.jsm:1037
getSessionPayload()
 TelemetrySession.jsm:1328
getPayload()
 TelemetrySession.jsm:1656
this.TelemetrySession<.getPayload()
 TelemetrySession.jsm:626
Impl.getCurrentPingData()
 TelemetryController.jsm:963
this.TelemetryController<.getCurrentPingData()
 TelemetryController.jsm:239
PingPicker._updateCurrentPingData()
 aboutTelemetry.js:327
PingPicker.update()
 aboutTelemetry.js:317
onLoad/<()
 aboutTelemetry.js:1463
onLoad()

> ::: toolkit/components/telemetry/Histograms.json:9086
> (Diff revision 3)
> > +    "n_values": 5,
> 
> make n_values=10 so it has room to grow. you won't be able to change the
> bucketing after you land the histogram

Will do.

Comment 17

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f262855e6972

Comment 18

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Comment 19

[Vladan Djeric (:vladan)]

> This patch did build and run... although I do see an error when I try loading about:telemetry.
> However, I still see this when running with a fixed build. Should I file a bug about these two 
> issues (the histogram problem not causing an error, and about:telemetry running into a JS error)?

Yes please! CC me as well

Comment 20

[Vladan Djeric (:vladan)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

https://reviewboard.mozilla.org/r/23601/#review21771

Comment 21

[:Margaret Leibovic]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #19)
> > This patch did build and run... although I do see an error when I try loading about:telemetry.
> > However, I still see this when running with a fixed build. Should I file a bug about these two 
> > issues (the histogram problem not causing an error, and about:telemetry running into a JS error)?
> 
> Yes please! CC me as well

Filed bug 1222042 and bug 1222044.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

https://reviewboard.mozilla.org/r/23601/#review21901

Comment 23

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/2586d921cb1fdffc54e9d7c41b3843ddaca7cfd2
Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2586d921cb1f

Comment 25

[:Margaret Leibovic]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Approval Request Comment
[Feature/regressing bug #]: None.

[User impact if declined]: We can't have this telemetry data until Firefox 45.

[Describe test coverage new/current, TreeHerder]: No automated tests, manual testing locally, landed on mozilla-central.

[Risks and why]: Low-risk. No logic changes, only adding telemetry histogram.

[String/UUID change made/needed]: None.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Adding a telemetry probe, should be safe enough. Let's uplift to Aurora44.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a47427a35e8c

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a47427a35e8c