Closed Bug 1392951 Opened 6 years ago Closed 6 years ago

Add Telemetry to understand how users set their home page

Categories

(Firefox :: Settings UI, defect, P3)

Product:
Firefox
Component:
Settings UI
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: chsiang, Assigned: evanxd)

References

(
URL
)

Details

(Whiteboard: [photon-preference])

User Story

[Background]
We would like to understand how users set their home page before we get into changing the design  

[Data Collected]
Send a ping when user type in “Home page” box
   - String is longer than 4 letters and with more than one “.”
   - String is not deleted after 3 seconds
Send a ping when “Use Current Page” is clicked
Send a ping when “Use Bookmark” is clicked
Send the number of “|” is detected in “Home page” box

[Purpose of Collection]
Understand if users change their home page
Understand how users change their home page
Understand how many home pages a user set

Attachments

(2 files)

Screen Shot 2017-08-23 at 4.20.33 PM.png
52.42 KB, image/png
Details
Bug 1392951 - Implement telemetry probe for home page setup.
59 bytes, text/x-review-board-request
liuche
: review+
mconley
: review+
ritu
: approval-mozilla-beta-
Details 
      No description provided.
Hi Chenxia, 
Could you help review the data collection of this bug?

Thanks.
Cindy
Flags: needinfo?(liuche)
(Commenting on User Story)
> [Background]
> We would like to understand how users set their home page before we get into
> changing the design  
> 
> [Data Collected]
> Send a ping when user type in “Home page” box
>    - String is longer than 4 letters and with more than one “.”
>    - String is not deleted after 3 seconds

Can you be specific about what goes up with this ping? Units, any data, etc

> Send a ping when “Use Current Page” is clicked
> Send a ping when “Use Bookmark” is clicked
> Send the number of “|” is detected in “Home page” box

What is this "|" for?

> 
> [Purpose of Collection]
> Understand if users change their home page
> Understand how users change their home page
> Understand how many home pages a user set


I'll have to see the documentation in-tree when this lands, but as long as the clarifications I requested aren't sending user-specific data up, this looks like Type 2 data, which should be fine.

Is this opt-out/opt-in, and what channels would it be on? Who is going to be responsible for looking at the data and deciding what to do with it, or when it no longer needs to be collected?
Flags: needinfo?(liuche) → needinfo?(chsiang)
Blocks: 1328561
Priority: -- → P3
Whiteboard: [photon-preferences] → [photon-preference][triage]
Target Milestone: --- → Firefox 57
Version: unspecified → 57 Branch
(In reply to Chenxia Liu [:liuche] from comment #2)
> (Commenting on User Story)
> > [Background]
> > We would like to understand how users set their home page before we get into
> > changing the design  
> > 
> > [Data Collected]
> > Send a ping when user type in “Home page” box
> >    - String is longer than 4 letters and with more than one “.”
> >    - String is not deleted after 3 seconds
> Can you be specific about what goes up with this ping? Units, any data, etc
>
This will be adding one count to the histogram of "type to add home page".
Conditions to send that +1 count can be discussed and modified - I will discuss with Ricky when he gets to this bug and update it again.
 
> > Send a ping when “Use Current Page” is clicked
> > Send a ping when “Use Bookmark” is clicked
> > Send the number of “|” is detected in “Home page” box
> 
> What is this "|" for?

When a user sets more than one sites as their home page, the home page box separates them with "|". And counting the number of "|" in home page box allows us to know how many sites a user sets as their home page without looking into the url.

> > 
> > [Purpose of Collection]
> > Understand if users change their home page
> > Understand how users change their home page
> > Understand how many home pages a user set
> 
> 
> I'll have to see the documentation in-tree when this lands, but as long as
> the clarifications I requested aren't sending user-specific data up, this
> looks like Type 2 data, which should be fine.
> 
> Is this opt-out/opt-in, and what channels would it be on? Who is going to be
> responsible for looking at the data and deciding what to do with it, or when
> it no longer needs to be collected?

Should be opt-out. Depending on the team's resource allocation, I would like to have this land in 57 Beta. 
I will be responsible for overseeing the data.
Flags: needinfo?(chsiang)
Whiteboard: [photon-preference][triage] → [photon-preference]
Flags: qe-verify-
Assignee: nobody → evan
Status: NEW → ASSIGNED
Attachment #8915870 - Flags: review?(mconley)
Attachment #8915870 - Flags: review?(liuche)
Hi Mike and Chenxia,

Could you help review the patch?

Thank you.
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

https://reviewboard.mozilla.org/r/187120/#review192552

::: browser/components/preferences/in-content/main.js:773
(Diff revision 6)
> +  onBrowserHomePageChange() {
> +    if (this.telemetryHomePageTimer) {
> +      clearTimeout(this.telemetryHomePageTimer);
> +    }
> +    let browserHomePage = document.querySelector("#browserHomePage").value;
> +    if (browserHomePage.length > 4 && /(\..*){2,}/.test(browserHomePage)) {

Can you please explain to me what's going on with this regex? It's really not clear.
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

https://reviewboard.mozilla.org/r/187120/#review192552

> Can you please explain to me what's going on with this regex? It's really not clear.

Sure, I've added a comment to explain the regex.
Hi Mike,

I've updated the patch to add a comment to explain the regex.
Could you help review it again?

Thank you very much.
Flags: needinfo?(mconley)
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

https://reviewboard.mozilla.org/r/187120/#review192852

data-review only. r+ with comment clarification addressed. This probe collects Type 2 usage data, and does not send any personally identifiable or privacy-sensitive information.

::: toolkit/components/telemetry/Scalars.yaml:562
(Diff revision 8)
> +    notification_emails:
> +      - chsiang@mozilla.com
> +    release_channel_collection: opt-out
> +    record_in_processes:
> +      - main
> +  browser_home_page_number:

To make this clearer, I'd slightly prefer something like "num_browser_home_page" or "browser_home_pages_set_count" or "browser_home_page_count" to be clearer, but up to you. Definitely need to change the comment below though to be explicit.

::: toolkit/components/telemetry/Scalars.yaml:566
(Diff revision 8)
> +      - main
> +  browser_home_page_number:
> +    bug_numbers:
> +      - 1392951
> +    description: >-
> +      Each key is the home page URL number when users change their home page,

Can you change this "number of home page urls" instead of home page URL number, and explicitly describe that multiple home pages can be set, delineated with a "|"?

We should be clear in this documentation especially because it deals with user urls, and the description is confusing and I had to go to the code to understand what was happening.
Attachment #8915870 - Flags: review?(liuche) → review+
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

https://reviewboard.mozilla.org/r/187120/#review193212

Thanks for the explanation!

::: browser/components/preferences/in-content/main.js:775
(Diff revision 8)
> +      clearTimeout(this.telemetryHomePageTimer);
> +    }
> +    let browserHomePage = document.querySelector("#browserHomePage").value;
> +    // The length of the home page URL string should be more then four,
> +    // and it should contain at least one ".", for example, "https://mozilla.org".
> +    if (browserHomePage.length > 4 && /(\..*){1,}/.test(browserHomePage)) {

Okay, I understand. In that case, can you use this (since it's a bit more readable):

```js

if (browserHomePage.length > 4 && browserHomePage.includes(".")) {
  // ...
}
```
Attachment #8915870 - Flags: review?(mconley) → review+
Flags: needinfo?(mconley)
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

https://reviewboard.mozilla.org/r/187120/#review192852

> To make this clearer, I'd slightly prefer something like "num_browser_home_page" or "browser_home_pages_set_count" or "browser_home_page_count" to be clearer, but up to you. Definitely need to change the comment below though to be explicit.

Sure, let's use `browser_home_page_count`.

> Can you change this "number of home page urls" instead of home page URL number, and explicitly describe that multiple home pages can be set, delineated with a "|"?
> 
> We should be clear in this documentation especially because it deals with user urls, and the description is confusing and I had to go to the code to understand what was happening.

Sure, let's do it.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4492a71092e
Implement telemetry probe for home page setup. r=liuche,mconley
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

Approval Request Comment
[Feature/Bug causing the regression]: None.
[User impact if declined]: No user impact, but we need the telemetry data to re-design the home page setup.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No need.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This change is only adding telemetry probe.
[String changes made/needed]: No.
Attachment #8915870 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/c4492a71092e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Comment on attachment 8915870 [details]
Bug 1392951 - Implement telemetry probe for home page setup.

I am not sure if this is the time to add a new telemetry probe in Beta57 cycle. Is this something that can wait and ride the 58 beta train?
Attachment #8915870 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.