1367125 - Record search cohort using the new annotation API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

In bug 1348748, we implemented a new API to properly annotate the different experiments in the environment data.

An interesting data point to add to this is the "search cohort".

This lists relevant code:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2F+searchCohort&redirect=false

The relevant test is run using:
./mach toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js

The experiment annotation API is documented here:
https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/index.html#experiment-annotation

For this bug we need to:
- Cu.import() TelemetryEnvironment.jsm into Experiments.jsm
- use the annotation API to mark the search cohort
- add test coverage
- do a simple manual test of the changes (i will detail this later after the rest works)

Comment 1

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> For this bug we need to:
> - Cu.import() TelemetryEnvironment.jsm into Experiments.jsm

Strike that, we just need to use the annotations API inside TelemetryEnvironment.jsm.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Florian, is the pref "browser.search.cohort" still actively used?

Comment 3

[Florian Quèze [:florian]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Florian, is the pref "browser.search.cohort" still actively used?

Yes, I think so. Or if it's not actively used currently, it's likely to be in the future. We use this preference whenever we are testing a new potential default search engine in a specific geography. Mike Kaply can tell you more about it if needed.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks.
Alejandro, do you want to take this bug?

Comment 5

[Alejandro Rodriguez Salamanca [:alejandro]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Thanks.
> Alejandro, do you want to take this bug?

Yes, I'll work on it.

Comment 6

[Mike Kaply [:mkaply]]

Yes, we definitely still actively use this.

Comment 7

[Georg Fritzsche [:gfritzsche]]

This is currently free to work on.

Comment 8

[Michael Hoffmann]

Hi, I'd like to work on this one.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Sure, great!
Do you need any further information on this?

Comment 10

[Michael Hoffmann]

Great!
Yes, I wonder if have I understood this correctly.
That is, I need to add methods to TelemetryEnvironment which will set and unset the searchCohort, and then add tests to test_TelemetryEnvironment.js.

Is that correct?

Comment 11

[Chris H-C :chutten]

Mostly correct! Near as I can tell you need only annotate the experiment information[1] with the searchCohort whenever that information is changed in TelemetryEnvironment's _updateSearchEngine function[2].

Does this make sense?

(Sorry for the long time between replies. When asking a question of a specific person we use the "Need more information from" field to make sure the question isn't missed in the bulk of bugmail we receive :S )

[1]: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/experiments.html
[2]: https://dxr.mozilla.org/mozilla-central/search?q=_updateSearchEngine&redirect=true

Comment 12

[Michael Hoffmann]

Thanks Chris,

This clarifies things, but I still wonder if I got it right.  I see that the `_updateSearchEngine` function does already set a searchCohort[1] under settings.  But with my updated understanding we should also set the searchCohort to be part of an experiment, with `TelemetryEnvironment.setExperimentActive('searchCohort', <value>)`.  Is that accurate?

Sorry for the delay, despite the needinfo, I had missed it but got a reminding email :-)

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#1182

Comment 13

[Chris H-C :chutten]

No worries. That is my understanding as well: have the searchCohort be an experiment.

Then you will need to add a test to ensure that it is set properly (and, if it changes, that it updates properly and we don't see two experiments with the same name or anything else strange)

Comment 14

[Michael Hoffmann]

Attachment: bug1367125.patch

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8886890 [details] [diff] [review]
bug1367125.patch

Review of attachment 8886890 [details] [diff] [review]:
-----------------------------------------------------------------

Chris, can you take a look here?

Comment 16

[Chris H-C :chutten]

Comment on attachment 8886890 [details] [diff] [review]
bug1367125.patch

Review of attachment 8886890 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7514d624a816
Record search cohort using the new annotation API. r=gfritzsche

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7514d624a816