Closed
Bug 1184705
Opened 9 years ago
Closed 9 years ago
Search A/B testing cohort identifier should be recorded in FHR
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 42
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file, 3 obsolete files)
9.68 KB,
patch
|
florian
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Due to the uncertainty about whether Unified Telemetry will be enabled in Firefox 40, I've been told this needs to be implemented in both FHRv2 and FHRv4. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a4c602b6ae6
Attachment #8634904 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 1•9 years ago
|
||
Comment on attachment 8634904 [details] [diff] [review] Patch I'm hearing gfritzsche is on PTO, so redirecting the review to gps/rnewman, whoever can get to it first :-). Needinfo on vladan for the data-collection review.
Flags: needinfo?(vdjeric)
Attachment #8634904 -
Flags: review?(rnewman)
Attachment #8634904 -
Flags: review?(gps)
Attachment #8634904 -
Flags: review?(gfritzsche)
Comment 2•9 years ago
|
||
Comment on attachment 8634904 [details] [diff] [review] Patch Review of attachment 8634904 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +885,5 @@ > + > + // Record the cohort identifier used for search defaults A/B testing. > + const kCohortPref = "browser.search.cohort"; > + if (Services.prefs.prefHasUserValue(kCohortPref)) > + this._currentEnvironment.settings.searchCohort = Services.prefs.getCharPref(kCohortPref); add this field to the Telemetry docs http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/ ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +1103,5 @@ > + > + // ... but that if a cohort identifier is set, we send it. > + Services.prefs.setCharPref("browser.search.cohort", "testcohort"); > + Services.obs.notifyObservers(null, "browser-search-service", "init-complete"); > + data = TelemetryEnvironment.currentEnvironment; are you ok with getting pings that don't have this field because the search service didn't finish initializing before the ping was created? e.g. very short sessions
Attachment #8634904 -
Flags: feedback+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2) > ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js > @@ +1103,5 @@ > > + > > + // ... but that if a cohort identifier is set, we send it. > > + Services.prefs.setCharPref("browser.search.cohort", "testcohort"); > > + Services.obs.notifyObservers(null, "browser-search-service", "init-complete"); > > + data = TelemetryEnvironment.currentEnvironment; > > are you ok with getting pings that don't have this field because the search > service didn't finish initializing before the ping was created? e.g. very > short sessions I think this is OK, as long as it's possible to filter out these pings when looking at the data (and I think it will be easy, as pings created before the search service is done initializing will also not contain the information related to the default search engine).
Updated•9 years ago
|
Flags: needinfo?(vdjeric)
Updated•9 years ago
|
QA Contact: petruta.rasa
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2) > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm > @@ +885,5 @@ > > + > > + // Record the cohort identifier used for search defaults A/B testing. > > + const kCohortPref = "browser.search.cohort"; > > + if (Services.prefs.prefHasUserValue(kCohortPref)) > > + this._currentEnvironment.settings.searchCohort = Services.prefs.getCharPref(kCohortPref); > > add this field to the Telemetry docs > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/ > docs/ Added in environment.rst.
Attachment #8634904 -
Attachment is obsolete: true
Attachment #8634904 -
Flags: review?(rnewman)
Attachment #8634904 -
Flags: review?(gps)
Attachment #8636035 -
Flags: review?(rnewman)
Attachment #8636035 -
Flags: review?(gps)
Attachment #8636035 -
Flags: review?(gfritzsche)
Comment 5•9 years ago
|
||
Comment on attachment 8636035 [details] [diff] [review] Patch v2 Review of attachment 8636035 [details] [diff] [review]: ----------------------------------------------------------------- Lift kCohortPref to the top level, call it PREF_SEARCH_COHORT. Otherwise, lgtm, and thanks for writing tests!
Attachment #8636035 -
Flags: review?(rnewman)
Attachment #8636035 -
Flags: review?(gps)
Attachment #8636035 -
Flags: review?(gfritzsche)
Attachment #8636035 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8636035 -
Attachment is obsolete: true
Attachment #8636077 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8636077 [details] [diff] [review] Patch v3 (as pushed) Approval Request Comment [Feature/regressing bug #]: search A/B testing (bug 1175218). [User impact if declined]: none. Business impact is that we wouldn't be able to collect the results from search experiments. [Describe test coverage new/current, TreeHerder]: the patch contains tests. [Risks and why]: low risk. This is just recording in FHR/Telemetry the value of a preference. [String/UUID change made/needed]: none.
Attachment #8636077 -
Flags: approval-mozilla-beta?
Attachment #8636077 -
Flags: approval-mozilla-aurora?
Comment 9•9 years ago
|
||
This does look to be low risk but I would still rather this land on m-c and that the data be verified before uplift. Let's consider this for beta7.
Comment 10•9 years ago
|
||
Backed out for browser_urlbar_search_healthreport.js failures. https://treeherder.mozilla.org/logviewer.html#?job_id=3839701&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/bcb1945e390e
Assignee | ||
Comment 12•9 years ago
|
||
Relanded with the straightforward browser_urlbar_search_healthreport.js fix.
Attachment #8636077 -
Attachment is obsolete: true
Attachment #8636077 -
Flags: approval-mozilla-beta?
Attachment #8636077 -
Flags: approval-mozilla-aurora?
Attachment #8636218 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8636218 [details] [diff] [review] Patch v3.1 (as relanded) Approval Request Comment: see comment 8
Attachment #8636218 -
Flags: approval-mozilla-beta?
Attachment #8636218 -
Flags: approval-mozilla-aurora?
Comment 14•9 years ago
|
||
Is manual QA coverage needed for this fix, considering that it is also covered by automated tests? If so, how could this be checked? Thanks!
Assignee | ||
Comment 15•9 years ago
|
||
I don't see any straightforward way to test that wouldn't just duplicate what the unit tests do. A 'real' manual test would involve setting up a server that sends a cohort, and somehow tweaking the URL to connect to that server instead of the official one. I don't think it's worth the effort.
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58398a0ad34e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 18•9 years ago
|
||
Comment on attachment 8636218 [details] [diff] [review] Patch v3.1 (as relanded) Now that this has successfully landed on m-c and Florian has confirmed that there is not a good way to manually verify, let's get this change in support of search experiments into beta7. Beta+ Aurora+
Attachment #8636218 -
Flags: approval-mozilla-beta?
Attachment #8636218 -
Flags: approval-mozilla-beta+
Attachment #8636218 -
Flags: approval-mozilla-aurora?
Attachment #8636218 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1fdd8c5ee97
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•