Closed Bug 1219240 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: Reader View, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Bug 1219240 - Telemetry probe to measure % of pages that show reader view button. r=Gijs
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs)
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.
Product: Firefox for Android → Toolkit
Version: Firefox 35 → Trunk
Component: General → Reader Mode
(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)...
Flags: needinfo?(margaret.leibovic)
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs) → review+
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.
(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?
Flags: needinfo?(margaret.leibovic) → needinfo?(bbermes)
(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.
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.
Flags: needinfo?(bbermes)
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
Attachment #8680246 - Attachment description: MozReview Request: Bug 1219240 - Telemetry probe to measure % of pages that show reader view button. r=Gijs → MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle
Attachment #8680246 - Flags: review?(mark.finkle)
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.
Component: Reader Mode → Reader View
Product: Toolkit → Firefox for Android
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.
Attachment #8680246 - Flags: review+ → review?(vladan.bugzilla)
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.
Attachment #8680246 - Flags: review?(mark.finkle)
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
Attachment #8680246 - Flags: review?(vladan.bugzilla)
Attachment #8680246 - Flags: review?(mark.finkle)
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs)
(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 on attachment 8680246 [details]
MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan

Stupid mozreview...
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs) → review?(vladan.bugzilla)
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": "mleibovic@mozilla.com",

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
Attachment #8680246 - Flags: review?(vladan.bugzilla)
(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": "mleibovic@mozilla.com",
> 
> 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.
Flags: needinfo?(vladan.bugzilla)
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
Attachment #8680246 - Attachment description: MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle → MozReview Request: Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan
Attachment #8680246 - Flags: review?(vladan.bugzilla)
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs)
Attachment #8680246 - Flags: review?(gijskruitbosch+bugs)
> 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
Flags: needinfo?(vladan.bugzilla)
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
Attachment #8680246 - Flags: review?(vladan.bugzilla) → review+
(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 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
Attachment #8680246 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/fx-team/rev/2586d921cb1fdffc54e9d7c41b3843ddaca7cfd2
Bug 1219240 - Telemetry probe to measure user interaction with reader view button. r=mfinkle,vladan
https://hg.mozilla.org/mozilla-central/rev/2586d921cb1f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Attachment #8680246 - Flags: approval-mozilla-aurora?
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.
Attachment #8680246 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1266163
You need to log in before you can comment on or make changes to this bug.