To monitor telemetry experiments and measure the results, we need to include in the FHR payload details about when the experiment was active. gps needs to clarify whether we need to adopt FHR v3 in order to accomplish this or whether we can stick with FHR v2 for this cycle. If v2, we need to record this in the daily structure. For v3 we'd record this as part of the session data.

In particular, knowing the - success rate - drop out rate - start times is important, and helps expose problems. (real world story: in a previous study, our Germany deploy was wrong, because we expected locale DE-DE, not DE. Seeing an uptake of '0' sooner would have alerted us to the problem.)

In more detail: FHR payload: * presence and active days (sessions) when an experiment is enabled Telemetry payload: * Log experiments that we considered: if a condition didn't match (or the JS function threw an exception) log the reason that the experiment was not accepted * Log experiment install/success/failure. * Log user-disable of an experiment. * Log normal experiment termination (via script from the experiment itself, or via expiration of the experiment).

If we stick with v2, we won't be able to reliably use data generated on days that experiments were activated/deactivated because we won't know with any certainty whether the data was collected with or without the experiment active (the experiment could have been activated at midnight and deactivated at 23:59.59). Throwing data away is annoying at best and harmful to the experiment at worst. For example, data on "border days" may mask significant changes of behavior corresponding to side-effects from the experiments being activated or deactivated. I would think that this data is very important to experiment runners and they'd hate to lose it. As this feature grows in popularity, the number of experiments being activated/deactivated will increase. Thus, the ratio of "border days" to non-border days will only increase. This will further undermine the value of the data. I would like to put off v3 for a later date because it bloats the scope of Telemetry Experiments and I'm all about avoiding scope bloat. But given all the benefits of v3, I think we should implement v3 for Telemetry Experiments. That being said, I'm still flushing out the requirements for the initial Telemetry Experiments experiment. It's quite possible we may go with a one-off for it so it can hit this release cycle. Then, we'll follow up with a more generic feature done right.

I think we still need to decide on v2 vs. v3 here.

v2. I discussed this with gps over the phone; sorry it never got recorded here.

However, we'll need to migrate the database schema to support releasing data belonging to providers that are no longer active. In current FHR, once an experiment's XPI is deactivated, FHR will no longer submit data belonging to that experiment. Since FHR overwrites records on the server, people interested in experiment data would have to extract server-side data before the experiment is deactivated. That's not cool.

Well, we don't technically need to change the schema. But there are other caveats. rnewman is in the office. I'll try to grab him to talk about it.

(In reply to Gregory Szorc [:gps] from comment #6) > In current FHR, once an experiment's XPI is deactivated, > FHR will no longer submit data belonging to that experiment. I thought we would be collecting the telemetry experiments info from the experiments API and add this as a specialized payload?

Ok, we were talking about different things here. Comment 7 was about the submission of the experiments data.

This will be a push provider in the core, an example for usage of push providers is found here: services/sync/modules/healthreport.jsm

> However, we'll need to migrate the database schema to support releasing data > belonging to providers that are no longer active. For v1, per the meeting a couple weeks ago, this is not a requirement. We can run the experiment for an extra week, save a snapshot of the data, and then not worry about it. Long-term we'll probably have incremental session upload and the whole problem becomes moot. The only persistent measurements we need now are the enabled status of an experiment.

Gregg will take a look here for now, i'll tackle the telemetry parts in bug 983360.

Reading this over, I am actually a bit confused here :) 1. Addon presence is shown in FHR already (only on current day?) 2. Should this data go to FHR at all, or just to telemetry? Privacy concerns in FHR? 3. If in FHR, what goes? What are the tuples? OPINION: Putting this in FHR seems weird to me, and we should think on this a bit more. (Weak) Proposal: new provider `ExperimentsProvider` ("org.mozilla.experiments") in `services/healthreport/providers.jsm`, with these fields: * FHRDAY, * experiment_info_json, * status_change, * reason This seems "not awesome", and most of the data will be "rejections", which feels data heavy.

(In reply to Gregg Lind (User Research - Test Pilot) from comment #13) > Reading this over, I am actually a bit confused here :) > > 1. Addon presence is shown in FHR already (only on current day?) Yes, only on current day which won't work for this case. The goal is to have the *only* presence of an experiment in the days array so that we can know which days it was active. The rest of the data is going into telemetry.

1. I am still not sure what to actually record here, and there are tons of edge cases here. Since this modifies the FHR packet, I am tempted to put this off for another rev, until we can design something useful. 2. Legal or privacy concerns of putting this in the fhr packet (I don't believe there are any) 3. Storage size concerns? 4. Payload format? 5. Is there a flag for knowing whether experiments is on or not. (In my ideal world, we would be recording success and failure via a pingback mechanism, for immediate logging and aggregation. FHR feels weird to me for this still.)

(In reply to Gregg Lind (User Research - Test Pilot) from comment #15) > 5. Is there a flag for knowing whether experiments is on or not. Depending on what exactly you want to do: * use the public API (Experiments.enabled) * watch the prefs (experiments.enabled)

The current FHR days packet is something like this: "2014-03-13": { "org.mozilla.addons.counts": { "_v": 2, "extension": 16, "plugin": 35, "theme": 12 }, "org.mozilla.healthreport.submissions": { "_v": 2, "continuationUploadAttempt": 1, "uploadSuccess": 1 }, ... }, The *only* data we should add to FHR as part of this bug is one additional element: "org.mozilla.org.telemetry-experiment": { "active": "experiment-id" } If there is no active experiment, this key should be omitted for that day. As noted in comment 2, the rest of the data is going into the telemetry payload and was broken off into bug 983360. There are no outstanding privacy/legal/size concerns.

To get notified of changes in the experiments activity status, you should also the public API from bug 974009. You need to listen to the "experiments-changed" topic.

Thank you for re-scoping this, and I can implement that. New Implementation plan: ADD DAILY datum. "org.mozilla.org.telemetry-experiment": { "active": "experiment-id" } This provider will appear IF AND ONLY IF and experiment was active at ANY POINT during that day.

Ben, I think "lastActive" is a good compromise here that allows future expansion. 1. Should I throw() if the conditions / contract around experiments are wrong? If so, throw what? 2. is onStartup/onShutdown the right way to add the listeners? If there is a Known Pattern for this, tell me.

Comment on attachment 8391295 [details] [diff] [review] 974024.fhr.v1.diff gps or maybe rnewman is probably best to answer that.

Comment on attachment 8391295 [details] [diff] [review] 974024.fhr.v1.diff Review of attachment 8391295 [details] [diff] [review]: ----------------------------------------------------------------- You'll need to document the new data in services/healthreport/docs/dataformat.rst. You should probably put the provider next to whatever code it is measuring. Inside Experiments.jsm is acceptable. This was the original intent of the FHR providers and the reason providers.jsm is an historical by-product. You'll need to update an XPCOM component manifest to register the new provider with FHR. See https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/healthreport/architecture.html#registering-providers ::: services/healthreport/providers.jsm @@ +1010,5 @@ > + version: 1, > + > + fields: { > + lastActive: Metrics.Storage.FIELD_DAILY_LAST_TEXT > + //active: Metrics.Storage.FIELD_DAILY_DISCRETE_TEXT lastActive is redundant. Fields must not change types. I would leave the name as "active." I think you should leave this as a discrete list because if multiple experiments are active for a day, we shouldn't lose that information. That will necessitate querying the database as part of updating to ensure you don't insert multiple identical values (we don't have the concept of a set type yet so you have to implement it yourself). @@ +1032,5 @@ > + // 1. 'get' the running experiment -- ?? > + // 2. listen for changes -- add listener > + > + onStartup: function (){ > + // add listener for experiment changes? Pretty sure this will trigger an error because it doesn't return a promise. (And yes, this is where you add the listener.) @@ +1036,5 @@ > + // add listener for experiment changes? > + }, > + > + onShutdown: function () { > + // remove listener for experiment changes? Ditto. @@ +1041,5 @@ > + }, > + > + collectConstantData: function () { > + // 1. one-off get the state. track changes by watching messages > + return this._collectAndStoreExperiments(); collectConstantData() is only called once per process lifetime. You want daily data instead since the experiment list is dynamic. Also, daily collections can occur more than once daily. @@ +1057,5 @@ > + */ > + return that.recordExperimentActivation(experiments[0].id) > + } > + }); > + return deferred.promise; Use a task here. @@ +1071,5 @@ > + /* > + monitorExperimentActivationChanges: function () { > + const { addObserver, removeObserver, notifyObservers } = > + Cc['@mozilla.org/observer-service;1'].getService(Ci.nsIObserverService); > + } Use Services.obs.

> I think you should leave this as a discrete list because if multiple > experiments are active for a day, we shouldn't lose that information. I told Gregg just to do the single scalar. On the analysis end we're not going to use anything more anyway. The simple thing for whatever experiment was last that day is good enough.

See attached Jetpack-style test. Need to translate these into core tests.

Comment on attachment 8394236 [details] [diff] [review] 974024.fhr.v2.diff Review of attachment 8394236 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really knowledgable on the FHR implementation, but here is commenting on other parts. ::: browser/experiments/Experiments.jsm @@ +1423,5 @@ > }; > + > + > + > +/* FHR Measumement per #974024 */ I don't think we need to point to a specific bug in a comment. If needed we have hg history (e.g. hg blame). @@ +1480,5 @@ > + > + observe: function (subject, topic, data) { > + let field; > + switch (topic) { > + case "experiments-changed": Use OBSERVER_TOPIC. @@ +1481,5 @@ > + observe: function (subject, topic, data) { > + let field; > + switch (topic) { > + case "experiments-changed": > + return this.recordLastActiveExperiment(null); What is the return needed for? @@ +1489,5 @@ > + collectDailyData: function () { > + return this.recordLastActiveExperiment.bind(this); > + }, > + > + promiseMostRecentActiveExperiment: function () { Nit: Do we actually need to name-prefix with "promise"? A "get" prefix (and if needed a comment on returning a promise) should be enough. @@ +1494,5 @@ > + let deferred = Promise.defer(); > + Experiments.instance().getExperiments().then(function (experimentsArray) { > + if (experimentsArray.length >= 0) { > + // should this cycle through active ones only? > + // can inactive ones be in here? Yes, both active & inactive experiments can be in there (the historical data). You cycle through the entries until the first one where active==false. Is it actually enough to only return one most recent active experiment? Could we have run more than one experiment since the last measurement? @@ +1499,5 @@ > + deferred.resolve(experimentsArray[0].id); > + } else { > + deferred.resolve(undefined); > + } > + }); If the promise return by getExperiments() is rejected, then your deferred doesn't get resolved. Maybe just use and return a task here instead (Task.spawn()) and get that problem fixed for free?

(In reply to Georg Fritzsche [:gfritzsche] from comment #26) > Is it actually enough to only return one most recent active experiment? > Could we have run more than one experiment since the last measurement? Ah, just saw comment 23, so never mind that part :)

(In reply to Georg Fritzsche [:gfritzsche] from comment #26) > @@ +1494,5 @@ > > + let deferred = Promise.defer(); > > + Experiments.instance().getExperiments().then(function (experimentsArray) { > > + if (experimentsArray.length >= 0) { > > + // should this cycle through active ones only? > > + // can inactive ones be in here? So, we discussed this shortly. As the list is in chronological order and contains historical data, we need to report the first entry iff: * it is active * its endDate is "today"

Revised addon-style test. I cannot figure out how to make this xpcshell-ish.

1. I this this incorporates most feedback, but it looks at the 'real' Experiments.instance(). I don't now who to decouple that, or if that is desired. 2. I am also not sure how to test it all, and would like if someone who knows more about testing FHR and xpishell tests to solve that. 3. Added in a more "assertive" bit on what `getExperiments` returns. gfritzsche and I disagree on this, and could use adjudication.

Comment on attachment 8394397 [details] [diff] [review] 974024.fhr.v3.diff Review of attachment 8394397 [details] [diff] [review]: ----------------------------------------------------------------- This is looking mostly good. We really need test coverage though. You should be able to write a standalone xpcshell test and not have to worry about modifying the state of the Experiments singleton instance. See services/sync/tests/unit/test_healthreport.js or any of the services/healthreport/tests/xpcshell/test_provider* for examples. ::: browser/experiments/Experiments.jsm @@ +376,5 @@ > + // 2. active experient if any, being first. > + let numactive = 0; > + let activeNotFirst = false; > + let L = list.length; > + for (let ii; ii < L; ii++) { Why ii instead of just i? @@ +377,5 @@ > + let numactive = 0; > + let activeNotFirst = false; > + let L = list.length; > + for (let ii; ii < L; ii++) { > + if (list[L].isActive) { Shouldn't you be accessing list[ii]? @@ +386,5 @@ > + } > + } > + if (numactive > 1 || activeNotFirst) { > + throw Error("getExperiments has more than one active experiment or the active experment is not in first position."); > + } What's this change have to do with adding an FHR provider? @@ +1448,5 @@ > + * > + * @return {Date} date at current UTC midnight > + */ > +let thisUTCMidnight = function (date) { > + if (date === undefined) {date = new Date(); } let thisUTCMidnight = function (date=newDate()) { let m = new Date(date); ... } Somewhere in experiments land I saw a prototype that held a _now() that could be monkeypatched to facilitate testing. We have an open bug against FHR on tests intermittently failing because the test is executing across a UTC day boundary. You'd be surprised how frequently that actually occurs! This function is susceptible to that intermittent failure. I would appreciate if you made an effort to eliminate that possibility. But that could be relegated to a follow-up bug (filed when the first intermittent failure occurs). @@ +1467,5 @@ > + name: "info", > + version: 1, > + > + fields: { > + lastActive: FIELD_DAILY_LAST_TEXT Nit: add trailing comma @@ +1480,5 @@ > + __proto__: Metrics.Provider.prototype, > + > + name: "org.mozilla.experiments", > + > + pullOnly: false, // uses observer, so must be kept alive. This is the default value, so you don't need it. @@ +1520,5 @@ > + }, > + > + getMostRecentActiveExperiment: function () { > + let deferred = Promise.defer(); > + let isTodayUTC = (d) => thisUTCMidnight(d).getTime() === thisUTCMidnight(Date.now()).getTime(); Nit: Double equals is fine. @@ +1528,5 @@ > + if (experimentsArray[0].active || isTodayUTC(new Date(experimentsArray[0].endDate))) { > + deferred.resolve(experimentsArray[0].id); > + } > + } else { > + deferred.resolve(undefined); Passing no arguments is the same as passing undefined. Well, sorta. For this use case it doesn't matter since you aren't looking at the special arguments variable, using rest arguments, etc. @@ +1550,5 @@ > + return mostRecent.then( > + function (id) { > + return m.setDailyLastText("lastActive", id); > + }.bind(this), > + function() Error("recordLastActiveExperiment failed because getMostRecentActiveExperiment rejected") Does this actually work? I didn't realize you could return Error instances from promise rejection handlers like that. Maybe you should write this as a Task to improve readability? ::: browser/experiments/Experiments.manifest @@ +2,5 @@ > contract @mozilla.org/browser/experiments-service;1 {f7800463-3b97-47f9-9341-b7617e6d8d49} > category update-timer ExperimentsService @mozilla.org/browser/experiments-service;1,getService,experiments-update-timer,experiments.manifest.fetchIntervalSeconds,86400 > category profile-after-change ExperimentsService @mozilla.org/browser/experiments-service;1 > + > +category healthreport-js-provider-default ExperimentsProvider resource://gre/browser/modules/Experiments/Experiments.jsm browser/modules/experiments/Experiments.jsm (note the lower case e)

gps can you take the tests? gregg is leaving imminently and doesn't really know how to fix them. > 3. Added in a more "assertive" bit on what `getExperiments` returns. gfritzsche and I disagree on this, and could use adjudication. I don't actually know what this means. gfritzsche can you explain?

Addressed my own nits and added unit tests. I don't think rnewman's review is strictly required. But he knows the FHR stuff, so it can't hurt to have him take a glance.

Comment on attachment 8394504 [details] [diff] [review] Add FHR recording of Telemetry Experiments activity Review of attachment 8394504 [details] [diff] [review]: ----------------------------------------------------------------- Assuming that I understand what you're doing, this looks good to me. ::: browser/experiments/Experiments.jsm @@ +5,5 @@ > "use strict"; > > this.EXPORTED_SYMBOLS = [ > "Experiments", > + "ExperimentsProvider" Nit: trailing comma. @@ +387,5 @@ > + }, > + > + /** > + * Obtain the entry of the most recent active experiment that was active > + * today. Assumption: that there's only one active experiment at a time. @@ +403,5 @@ > + return experiment; > + } > + > + if (experiment.endDate && this._dateIsTodayUTC(experiment.endDate)) { > + return experiment; Possible error: if one experiment *was* active today, and another is active now, but comes later in the experiment list, the former will be returned. This might be undesirable, but it should be documented regardless, no? If you address this -- e.g., always return the current active experiment over one that was active UTC-today -- then please add a test. @@ +1499,5 @@ > + > + name: "org.mozilla.experiments", > + > + measurementTypes: [ > + ExperimentsLastActiveMeasurement1 Trailing commas throughout. @@ +1525,5 @@ > + return Promise.resolve(); > + }, > + > + observe: function (subject, topic, data) { > + let field; Unused.

(In reply to Richard Newman [:rnewman] from comment #34) > Assumption: that there's only one active experiment at a time. That's by design for the whole feature. > @@ +403,5 @@ > > + return experiment; > > + } > > + > > + if (experiment.endDate && this._dateIsTodayUTC(experiment.endDate)) { > > + return experiment; > > Possible error: if one experiment *was* active today, and another is active > now, but comes later in the experiment list, the former will be returned. The list is sorted by endDate, most recent first. Currently active experiments will always have the most recent (future) endDate. |experiment.endDate| not being set or 0 would be an error, so this really only needs to grab the first entry, if any.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #32) > > 3. Added in a more "assertive" bit on what `getExperiments` returns. gfritzsche and I disagree on this, and could use adjudication. > > I don't actually know what this means. gfritzsche can you explain? I disagreed on adding additional runtime verification of the list ordering and state right after we build it up in |getExperiments()| (IMO should be something we add test coverage for etc.). I wasn't the reviewer and didn't want to spend too long discussing this, so i moved on.

(In reply to Georg Fritzsche [:gfritzsche] from comment #35) > > Assumption: that there's only one active experiment at a time. > > That's by design for the whole feature. Yeah, I was requesting that it be put in the comment for the benefit of the poor sucker who has to fix a bug in this in 2015 :) > The list is sorted by endDate, most recent first. Currently active > experiments will always have the most recent (future) endDate. > |experiment.endDate| not being set or 0 would be an error, so this really > only needs to grab the first entry, if any. Great, let's put this in a comment.

I forgot to add the test file. Derp. Will incorporate rnewman's comments soon. Should be a minimal diff (not that Splinter will render the interdiff properly).

Addressed rnewman's feedback.