Benjamin Smedberg Assignee
	Description • 9 years ago

Experiment filter functions have access to check historical FHR data. With unified telemetry the old API isn't going to work and needs to be fixed.

I really don't want the filter functions to become asynchronous. We could either pre-collect the entire FHR history and pass it to the filter, or we could move the filter onto a worker and expose some kind of synchronous API that way.

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 9 years ago

I think pre-collecting this is not feasible resource-wise.
Can you sketch out or point to examples how moving this to a worker would look like?

	Benjamin Smedberg Assignee
	Comment 2 • 9 years ago

I asked around about workers, and it appears that workers currently can't block on the main thread. We proposed an API to allow that, but it was never implemented.

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 9 years ago

Given that we don't have a working design here yet and 40 is closing in, i don't think this bug will realistically make it in time.

Do we actually use the jsfilter commonly for experiments?
Can we potentially live without it for a cycle?

	Benjamin Smedberg Assignee
	Comment 4 • 9 years ago

I think the only thing we must have is the current environment. That is used to (for example) disable an experiment for users who have a certain addon installed. And that should be available synchronously.

	Georg Fritzsche [:gfritzsche]
	Comment 5 • 9 years ago

Ok, that would be easy enough to provide.
We can just add a "currentTelemetryEnvironment" field here:
https://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/browser/experiments/Experiments.jsm#l1735

	Benjamin Smedberg Assignee
	Comment 6 • 9 years ago

Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

	Georg Fritzsche [:gfritzsche]
	Comment 7 • 9 years ago

Comment on attachment 8625504 [details]
MozReview Request: Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

https://reviewboard.mozilla.org/r/11837/#review10369

Nice surprise to see this get picked up!

::: browser/experiments/Experiments.jsm
(Diff revision 1)
> -const PREF_HEALTHREPORT_ENABLED = "datareporting.healthreport.service.enabled";

While this pref is unused here, we need to fix ExperimentsService.js to initialize this regardless of FHR being on or off.

::: browser/experiments/Experiments.jsm:1712
(Diff revision 1)
> -        result = !!Cu.evalInSandbox("filter({healthReportPayload: JSON.parse(_hr), telemetryPayload: JSON.parse(_t)})", sandbox);
> +      result = !!Cu.evalInSandbox("filter({get telemetryEnvironment() { return _e; } })", sandbox);

When we uplift to Beta, are we ok with requiring filter functions for experiments to switch over to using the environment data right now?

Is the currentEnvironment enough or would it be useful to get the current ping data too (TelemetryController.getCurrentPingData())?

	Benjamin Smedberg Assignee
	Comment 8 • 9 years ago

Comment on attachment 8625504 [details]
MozReview Request: Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

> > -const PREF_HEALTHREPORT_ENABLED = "datareporting.healthreport.service.enabled";
> 
> While this pref is unused here, we need to fix ExperimentsService.js to
> initialize this regardless of FHR being on or off.

Filed bug 1178456. I don't want to do that in this bug since it's mostly unrelated.

> When we uplift to Beta, are we ok with requiring filter functions for
> experiments to switch over to using the environment data right now?

Yes. None of the current or planned experiments have a filter.

> Is the currentEnvironment enough or would it be useful to get the current
> ping data too (TelemetryController.getCurrentPingData())?

I don't know of any use case where the current ping data is helpful.

	Georg Fritzsche [:gfritzsche]
	Comment 9 • 9 years ago

https://reviewboard.mozilla.org/r/11835/#review10743

I would say ship it, but i don't have any options for that anymore. Yay.

	Georg Fritzsche [:gfritzsche]
	Comment 10 • 9 years ago

Comment on attachment 8625504 [details]
MozReview Request: Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

https://reviewboard.mozilla.org/r/11837/#review10747

Ship It!

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/7a19600cbcf9

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7a19600cbcf9

	Georg Fritzsche [:gfritzsche]
	Comment 13 • 9 years ago

Comment on attachment 8625504 [details]
MozReview Request: Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

Approval Request Comment
[Feature/regressing bug #]: Unified Telemetry
[User impact if declined]: Telemetry Experiments won't be able to use the filter() function for selecting clients to run on.
[Describe test coverage new/current, TreeHerder]: Good automated test-coverage.
[Risks and why]: Low-risk, this is a small change, contained to a pretty specific part of the Experiments code.
[String/UUID change made/needed]: None.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 14 • 9 years ago

Comment on attachment 8625504 [details]
MozReview Request: Bug 1151086 - Fix experiment filter functions to use the new telemetry environment, r?gfritzsche

The patch has a lot of code change but given that it baked in m-c for 3 weeks, let's land it in Aurora now!

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/49e585f3ab9c

	Georg Fritzsche [:gfritzsche]
	Comment 16 • 9 years ago

(In reply to Ritu Kothari (:ritu) from comment #14)
> The patch has a lot of code change but given that it baked in m-c for 3
> weeks, let's land it in Aurora now!

Note that most of the changes are only in tests.