Closed Bug 1334617 Opened 8 years ago Closed 8 years ago

Consider making FX_URLBAR_SELECTED_RESULT_TYPE opt-out

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: past, Assigned: past)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files, 1 obsolete file)

Make the URL bar selected result telemetry probes opt-out (bug 1334617).
59 bytes, text/x-review-board-request
benjamin
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Revert "Bug 1334617 followup - Eslint loves commented code. a=tomcat"
59 bytes, text/x-review-board-request
mak
: review+
Details
Revert "Bug 1334617 - Temporarily disable tests for FX_URLBAR_SELECTED_RESULT_INDEX since it's expired. rs=Dexter, landing on central with a=tomcat"
59 bytes, text/x-review-board-request
mak
: review+
Details
It would be useful to make FX_URLBAR_SELECTED_RESULT_TYPE opt-out, if the data privacy review approves it. It doesn't contain any PII in my view.
This one is top priority
Priority: -- → P1
Assignee: nobody → past
Status: NEW → ASSIGNED
Benjamin, to give you some more context, I'm making two changes here: (a) removing the expiration from FX_URLBAR_SELECTED_RESULT_INDEX (b) making both that and FX_URLBAR_SELECTED_RESULT_TYPE opt-out I have read your comments in bug 775825 where you had argued against (a), but we now have a dedicated data analyst tasked to create dashboards for us and it's one of the top-level goals for both the search team and BD to monitor the awesomebar results and use them to improve their value for our users. In addition to the above, another argument for (b) is that it doesn't contain any PII, is going to be super useful for improving the awesomebar and Javaun tells me that it is covered by the data policy documentation Rebecca is putting together as something that is acceptable to have opt-out.
Javaun and I had a discussion about how the utility of the selected result index measurement could be improved if it could be associated with result type. Could we possibly turn FX_URLBAR_SELECTED_RESULT_INDEX into a keyed histogram, keyed by the values for result type, ie. URLBAR_SELECTED_RESULT_TYPES (https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUsageTelemetry.jsm#56)? We weren't sure what additional engineering effort or considerations that would entail.
Flags: needinfo?(past)
I'm happy to do this, but I wonder if repurposing an existing histogram will break our pipeline. Georg, should we create a new keyed histogram in this case or reuse the existing one? If a new one is necessary, should we keep the old one around (perhaps for a short period of time) or delete it?
Flags: needinfo?(past) → needinfo?(gfritzsche)
Changing histograms while keeping the same name is not supported, see [1]. Whether to keep the old definition around depends on the users. Removing it will make it disappear from e.g. TMO dashboards. 1: http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#changing-a-histogram
Flags: needinfo?(gfritzsche)
Attachment #8842469 - Flags: review?(benjamin)
Javaun, could you state for the record that you will be the long-term owner of looking at this data?
Flags: needinfo?(jmoradi)
note that this caused a perma failure with bug 1345134 - so i guess this should be fixed soon here
Blocks: 1345134
I'll push a temporary patch to comment out the parts of the test that refer to the expired probe. I just need time for a build and running the test locally to ensure it works.
Pushed by mak77@bonardo.net: https://hg.mozilla.org/mozilla-central/rev/8718138ada82 Temporarily disable tests for FX_URLBAR_SELECTED_RESULT_INDEX since it's expired. rs=Dexter, landing on central with a=tomcat
Pushed by mak77@bonardo.net: https://hg.mozilla.org/mozilla-central/rev/3d341b9ba535 followup - Eslint loves commented code. a=tomcat
Note: All the temporary disabling I'm doing should be undone by the patch that will land here.
Keywords: leave-open
BSmedberg: I'll be the long-term owner of this bug. I'll talk w/ DZeber about doing the data review paperwork. I'm eager to do my first so I can learn.
Flags: needinfo?(jmoradi)
You don't need the complex new-project paperwork. When the patch is ready here I can mark the data review.
It's surprisingly hard for me to find time to do the work suggested in comment 4, and since Javaun agreed that we need both the existing histograms and the new one, I will file a followup for comment 4 and use this bug for its original purpose.
Attachment #8845384 - Attachment is obsolete: true
Attachment #8845384 - Flags: review?(benjamin)
It was also surprisingly hard for me to figure out the proper syntax for 'git mozreview push', but YOLO!
Is it possible to have this uplifted to 53? I realize we're in Beta now. A funnelcake is being discussed and this would be a critical probe to understand some behavior they're testing in that test.
Comment on attachment 8842469 [details] Make the URL bar selected result telemetry probes opt-out (bug 1334617). https://reviewboard.mozilla.org/r/116298/#review121134 data-r+
Attachment #8842469 - Flags: review?(benjamin) → review+
Comment on attachment 8845385 [details] Revert "Bug 1334617 followup - Eslint loves commented code. a=tomcat" https://reviewboard.mozilla.org/r/118590/#review121136 This doesn't look like a data review. This is not my code so I don't want to do code review on this.
Attachment #8845385 - Flags: review?(benjamin)
Comment on attachment 8845386 [details] Revert "Bug 1334617 - Temporarily disable tests for FX_URLBAR_SELECTED_RESULT_INDEX since it's expired. rs=Dexter, landing on central with a=tomcat" https://reviewboard.mozilla.org/r/118592/#review121138
Attachment #8845386 - Flags: review?(benjamin)
Attachment #8845385 - Flags: review?(mak77)
Attachment #8845386 - Flags: review?(mak77)
Comment on attachment 8845385 [details] Revert "Bug 1334617 followup - Eslint loves commented code. a=tomcat" https://reviewboard.mozilla.org/r/118590/#review121590
Attachment #8845385 - Flags: review?(mak77) → review+
Comment on attachment 8845386 [details] Revert "Bug 1334617 - Temporarily disable tests for FX_URLBAR_SELECTED_RESULT_INDEX since it's expired. rs=Dexter, landing on central with a=tomcat" https://reviewboard.mozilla.org/r/118592/#review121592
Attachment #8845386 - Flags: review?(mak77) → review+
Pushed by pastithas@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/536676c30456 Make the URL bar selected result telemetry probes opt-out . r=bsmedberg https://hg.mozilla.org/integration/autoland/rev/59d830905c61 Revert "Bug 1334617 followup - Eslint loves commented code. a=tomcat" r=mak https://hg.mozilla.org/integration/autoland/rev/3862c0c5350c Revert "Bug 1334617 - Temporarily disable tests for FX_URLBAR_SELECTED_RESULT_INDEX since it's expired. rs=Dexter, landing on central with a=tomcat" r=mak
These changes landed, but somehow the bug wasn't updated: https://hg.mozilla.org/mozilla-central/rev/536676c30456 https://hg.mozilla.org/mozilla-central/rev/59d830905c61 https://hg.mozilla.org/mozilla-central/rev/3862c0c5350c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8842469 [details] Make the URL bar selected result telemetry probes opt-out (bug 1334617). Approval Request Comment [Feature/Bug causing the regression]: none [User impact if declined]: no user impact, but we count on these telemetry probes to inform planned implementation work [Is this code covered by automated tests?]: yes and it was briefly broken in nightly, before being properly fixed by this patch [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none, the other two patches aren't necessary in other branches [Is the change risky?]: no [Why is the change risky/not risky?]: it's a small configuration change in our data collection [String changes made/needed]: none
Attachment #8842469 - Flags: approval-mozilla-beta?
Attachment #8842469 - Flags: approval-mozilla-aurora?
Comment on attachment 8842469 [details] Make the URL bar selected result telemetry probes opt-out (bug 1334617). Has data review+, sounds like it will be useful for the product team. Let's bring it to 53.
Attachment #8842469 - Flags: approval-mozilla-beta?
Attachment #8842469 - Flags: approval-mozilla-beta+
Attachment #8842469 - Flags: approval-mozilla-aurora?
Attachment #8842469 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Panos' assessment on manual testing needs (see Comment 31) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: