Closed Bug 1184705 Opened 4 years ago Closed 4 years ago

Search A/B testing cohort identifier should be recorded in FHR

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch (obsolete) — 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)
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 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+
(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).
Flags: needinfo?(vdjeric)
QA Contact: petruta.rasa
Attached patch Patch v2 (obsolete) — Splinter Review
(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 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+
Attached patch Patch v3 (as pushed) (obsolete) — Splinter Review
Attachment #8636035 - Attachment is obsolete: true
Attachment #8636077 - Flags: review+
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?
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.
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+
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?
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!
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.
https://hg.mozilla.org/mozilla-central/rev/58398a0ad34e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Thank you, Florian! I'm adding the qe-verify- flag.
Flags: qe-verify-
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+
You need to log in before you can comment on or make changes to this bug.