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)
Tracking
()
RESOLVED
FIXED
Firefox 57
People
(Reporter: chsiang, Assigned: evanxd)
References
()
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)
52.42 KB,
image/png
|
Details | |
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)
Comment 2•6 years ago
|
||
(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)
Updated•6 years ago
|
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)
Updated•6 years ago
|
Whiteboard: [photon-preference][triage] → [photon-preference]
Updated•6 years ago
|
Flags: qe-verify-
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → evan
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8915870 -
Flags: review?(mconley)
Attachment #8915870 -
Flags: review?(liuche)
Assignee | ||
Comment 10•6 years ago
|
||
Hi Mike and Chenxia, Could you help review the patch? Thank you.
Comment 11•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 15•6 years ago
|
||
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 16•6 years ago
|
||
mozreview-review |
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 17•6 years ago
|
||
mozreview-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+
Updated•6 years ago
|
Flags: needinfo?(mconley)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4492a71092e Implement telemetry probe for home page setup. r=liuche,mconley
Assignee | ||
Comment 22•6 years ago
|
||
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?
![]() |
||
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c4492a71092e
status-firefox57:
--- → affected
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.
Description
•