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.

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?

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.

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?

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.

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

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

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())?

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.

https://reviewboard.mozilla.org/r/11835/#review10743 I would say ship it, but i don't have any options for that anymore. Yay.

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!

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.

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!

(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.