1367129 - Record e10s cohort using the new annotation API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

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 "e10s cohort".

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

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:
- in TelemetryEnvironment.jsm use the annotation API to mark the e10s cohort
- confirm tests are working
- do a simple manual test of the changes (i will detail this later after the rest works)

Comment 1

[tfe]

Hello. I am quite new in the firefox dev. I have already resolved 2 bugs, but never worked on the telemetry part already.
Can i work on this issue?

Comment 2

[Benjamin Smedberg]

Georg, why would we do the annotation in TelemetryEnvironment.jsm instead of in the e10s system addon? Doing it in the environment seems like a strange coupling.

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> Georg, why would we do the annotation in TelemetryEnvironment.jsm instead of
> in the e10s system addon? Doing it in the environment seems like a strange
> coupling.

We already have settings.e10sCohort in the environment data, this would just mirror it in the annotations.
https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/environment.html

Do you think this data point is not useful anymore?
Should we remove the e10sCohort instead?

Comment 4

[Benjamin Smedberg]

I agree that an experiment annotation is better than the old e10scohort marking, so I'm not saying we shouldn't fix this bug. However I'm asking why we would set this annotation from the telemetryenvironment code, instead of doing it directly from the system addon. That would have the benefit of decoupling the environment code from the e10s deployment, and be a model for other progressive deployments/ab tests.

Short-term we also have to keep e10scohort annotation because the e10s analyses are using it.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> I agree that an experiment annotation is better than the old e10scohort
> marking, so I'm not saying we shouldn't fix this bug. However I'm asking why
> we would set this annotation from the telemetryenvironment code, instead of
> doing it directly from the system addon. That would have the benefit of
> decoupling the environment code from the e10s deployment, and be a model for
> other progressive deployments/ab tests.

Gabor, do you have an opinion on this or can redirect to someone?
Would you know where and how to do this?

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to tfe from comment #1)
> Hello. I am quite new in the firefox dev. I have already resolved 2 bugs,
> but never worked on the telemetry part already.
> Can i work on this issue?

Thanks for the interest!
Let me settle the details here, then i can flag you here.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Actually setting the needinfo for comment 5.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> > I agree that an experiment annotation is better than the old e10scohort
> > marking, so I'm not saying we shouldn't fix this bug. However I'm asking why
> > we would set this annotation from the telemetryenvironment code, instead of
> > doing it directly from the system addon. That would have the benefit of
> > decoupling the environment code from the e10s deployment, and be a model for
> > other progressive deployments/ab tests.
> 
> Gabor, do you have an opinion on this or can redirect to someone?

I agree with Benjamin.

> Would you know where and how to do this?

The code you're probably looking for is at: http://searchfox.org/mozilla-central/source/browser/extensions/e10srollout/bootstrap.js

I'm not sure about the deployment procedure, you should talk to :felipe or :mrbkap if you have any specific questions.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Nice new API! I will definitely gonna direct people to use it on new experiments.

But in the case of e10s, tbh, I don't know if there's much value of adding it now, since there's a lot of dashboards and data analysis out there that's already using the existing annotation. If anyone thinks it would still be good to add, it would be a good-first-bug as it's just a matter of adding to the setCohort function here: http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/browser/extensions/e10srollout/bootstrap.js#227

Comment 10

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Add annotation API to record e10s cohort

Comment 11

[Alejandro Rodriguez Salamanca [:alejandro]]

Hmmm, I don't know why the file npm-shrinkwrap.json changed. I'll upload a new patch without those changes.

Comment 12

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Add annotation API to record e10s cohort

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8880844 [details] [diff] [review]
Add annotation API to record e10s cohort

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

Looks great! Thanks, Alejandro!

Comment 14

[Georg Fritzsche [:gfritzsche]]

Alejandro, is this good to land?
If yes, you can set the keyword "checkin-needed" for it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d839307ca6e
Add annotation API to record e10s cohort. r=felipe

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d839307ca6e

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880844 [details] [diff] [review]
Add annotation API to record e10s cohort

I would like to uplift this patch to Beta. As things stand, this is the only difference between Nightly and Beta (though the GoFaster update we pushed to Release recently did include it). I'd like the e10srollout addon to match on Nightly and Beta to make development and reasoning about the add-on easier.

This patch is also very low-risk and has been indirectly shipped to Release.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8880844 [details] [diff] [review]
Add annotation API to record e10s cohort

telemetry foo for e10srollout, for 55.0b13

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880844 [details] [diff] [review]
Add annotation API to record e10s cohort

After testing, I realized that this regresses bug 1372824 on beta. It's possible to fix that, but at this point we should probably be uplifting fewer patches given the late date in the release cycle. I'll fix e10srollout on Nightly and we'll let this ride the trains.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8880844 [details] [diff] [review]
Add annotation API to record e10s cohort

per discussion in #e10s this change would regress 1372824, so we'll keep it on 56 only.