1349214 - Telemetry Experiments should use new annotation API

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

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

To have actual usage of the API, we should make Experiments.jsm call this.

This should show the relevant points to add this:
https://dxr.mozilla.org/mozilla-central/search?q=EXPERIMENTS_CHANGED_TOPIC+path%3AExperiments.jsm&redirect=false

Test coverage goes into:
browser/experiments/test/xpcshell/

Comment 1

[Georg Fritzsche [:gfritzsche]]

Alessio, can you set this up for mentoring?

Comment 2

[Georg Fritzsche [:gfritzsche]]

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 when Telemetry Experiments are active/inactive
- add test coverage
- do a simple manual test of the changes (i will detail this later after the rest works)

The places where we need to add those calls are around the `notifyObserverCalls()` here:
https://dxr.mozilla.org/mozilla-central/search?q=EXPERIMENTS_CHANGED_TOPIC+path%3AExperiments.jsm&redirect=false

We should be able to add test coverage to this test:
https://dxr.mozilla.org/mozilla-central/source/browser/experiments/test/xpcshell/test_telemetry.js

That test already does starting and stopping of Telemetry Experiments and checks Telemetry results for this.
We can add uses of `TelemetryEnvironment.getActiveExperiments()` to check the annotations worked properly.

Comment 3

[Deepa]

Hi Georg,

I would like to take this up :-)

Comment 4

[Georg Fritzsche [:gfritzsche]]

Hi Deepa, were you still looking into this?

Comment 5

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Error in tests when trying to add the new annotation API

Comment 6

[Alejandro Rodriguez Salamanca [:alejandro]]

Comment on attachment 8880439 [details] [diff] [review]
Error in tests when trying to add the new annotation API

This patch shows that, when trying to add the new annotation API to Experiments.jsm, some tests fail. If test_getExperiments is run, the following error appears:

> test_getExperiments FAIL [test_getExperiments : 216] Experiments observer should have been called. - 5 == 4

This happens because Services.obs.notifyObservers(null, EXPERIMENTS_CHANGED_TOPIC); in line 1300 is called twice for the same experiment. After some debugging, the errors seems to be in this._firstEvaluate, that becomes true at some point.

Why this happens when the new annotation API is added is unclear, as it should not affect in the execution of this code.

Comment 7

[Jeremy Lempereur [:o0ignition0o]]

Hey there, can I pick this one up ? :)

Comment 8

[Jeremy Lempereur [:o0ignition0o]]

Ok so I've setup https://reviewboard.mozilla.org/r/209728/ as a draft but I cannot publish it (Probably because Georg is away until Jan the 2nd) I'll try to publish it tomorrow :)

I've added a new log type I called BRANCH_SWITCH, and I'm not really sure I'm calling the setExperiment[Active/Inactive] at the right spots, so this will probably require some bikeshed ^^'

I wonder if I could add other relevant xpcshell tests to make sure the fix is working as requested ?

Oh and a happy new year ! \o/

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Jeremy Lempereur from comment #8)
> Ok so I've setup https://reviewboard.mozilla.org/r/209728/ as a draft but I
> cannot publish it (Probably because Georg is away until Jan the 2nd) I'll
> try to publish it tomorrow :)
> 
> I've added a new log type I called BRANCH_SWITCH, and I'm not really sure
> I'm calling the setExperiment[Active/Inactive] at the right spots, so this
> will probably require some bikeshed ^^'

Sure, publish the review request and flag Georg, he should be available now :)
I wonder though if that's still relevant due to bug 1415284.

Georg, should we try to fix this or drop it in favour of bug 1415284?

> Oh and a happy new year ! \o/

Thamks, happy new year to you too!

Comment 10

[Jeremy Lempereur [:o0ignition0o]]

Hey Alessio :)

I just tried to publish the review again but it seems Georg is hasn't switched the "accept reviews" flag yet ^^ 

` Error publishing: Bugzilla error: Georg Fritzsche [:gfritzsche] [away Dec 22 - Jan 2] <[e-mail]> is not currently accepting 'review' requests. `

I'm just going to wait a bit more.

After reading about https://bugzilla.mozilla.org/show_bug.cgi?id=1415284 I feel like this one is not relevant anymore, maybe I should rather work on it instead ?

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #9)
> (In reply to Jeremy Lempereur from comment #8)
> > Ok so I've setup https://reviewboard.mozilla.org/r/209728/ as a draft but I
> > cannot publish it (Probably because Georg is away until Jan the 2nd) I'll
> > try to publish it tomorrow :)
> > 
> > I've added a new log type I called BRANCH_SWITCH, and I'm not really sure
> > I'm calling the setExperiment[Active/Inactive] at the right spots, so this
> > will probably require some bikeshed ^^'
> 
> Sure, publish the review request and flag Georg, he should be available now
> :)
> I wonder though if that's still relevant due to bug 1415284.
> 
> Georg, should we try to fix this or drop it in favour of bug 1415284?

We are definitely removing Telemetry Experiments and they are not actively used anymore.
So we should probably close this bug and focus on other issues :)

Comment 12

[Jeremy Lempereur [:o0ignition0o]]

Sure, if there's any other issue you would like to mentor me on, please ping / assign me ! :)

Comment 13

[Alessio Placitelli [:Dexter]]

Let's close this due to bug 1415284 :) Jeremy, Georg will follow-up with you on interesting next bugs :)