mozilla / nimbus-shared

Shared data and schemas for Project Nimbus
https://mozilla.github.io/nimbus-shared
Mozilla Public License 2.0
6 stars 15 forks source link

Remove dates from Rapid Experiment Recipe #56

Open jaredlockhart opened 4 years ago

jaredlockhart commented 4 years ago

Currently we have startDate, endDate, proposedEnrollment included as fields in the experiment schema that will be deployed to remote settings here:

https://github.com/mozilla/nimbus-shared/blob/main/types/experiments.ts#L41-L48

As I'm working on the deployment workflow I realize it will be difficult to keep these dates accurate and in sync with the dates that are stored in Experimenter, example:

1) Create experiment and request review on Day 1 2) Recipe is generated capturing Day 1 as startDate and pushed to remote settings 3) Recipe sits in review for 1 whole day before it is reviewed and approved on Day 2 4) Experimenter captures the 'actual' start date as Day 2 5) Experimenter's API exposes the experiment start date as Day 2 but the recipe in remote settings shows Day 1

Keeping these things in sync doesn't seem practical (there's no way to update the remote settings data after the fact without also going through another dual sign off which just repeats the same problem), so another possibility is to omit the dates from the recipe altogether and only store and expose them from Expermienter's API so there's a single source of truth for them.

Thoughts? @k88hudson @tdsmith @mythmon ?

mythmon commented 4 years ago

Drawing on Normandy experience, I think this is a good idea. Normandy has two kinds of recipe serializers: the "standard" serializer that includes all the details like enabled date and author, and the "minimal serializer" that only includes the data that is needed to execute the recipe. The full serializer is used in the API that tools interact with, and the minimal serializer is published for executing clients (both via an API and via Remote Settings). The goals for this in Normandy were different, but I think the technique would work well here too.

This raises the problem of code duplication to me. I think that the type published to remote settings should be a subset of the type in the Experimenter API. Doing this by hand would be annoying and troublesome, but we could use some of Typescripts tools to do it, specifically one of Pick or Omit.

interface Experiment {
  slug: string;
  startDate: string;
  endDate: string;
  ...
}

// These are both equivalent to `interface MinimalExperiment { slug: string; ... }`
type MinimalExperiment1 = Omit<Experiment, 'startDate' | 'endDate'>;
type MinimalExperiment2 = Pick<Experiment, 'slug' | ...>;
k88hudson commented 4 years ago

I think that approach @mythmon suggests sounds good, alternatively having Experiment represent a minimal set of properties and ExperimentExtendedAPI include extra ones (although I think the Omit approach is probably right).

It sounds fine to me not to include start/end/proposed enrollment in RS if they aren't needed for about:studies or anything else

jaredlockhart commented 4 years ago

Oh that's a neat idea @mythmon ! I'd be in favour of using Pick over Omit so that it's very explicit which fields are included in which type and you don't accidentally introduce fields in types you didn't expect them in. Also having one be a strict superset of the other seems like the right approach for now, though it's not impossible that they might sufficiently diverge in the future that that no longer becomes viable.

mythmon commented 4 years ago

Is anyone working on this? I haven't been very involved in the actual definitions of schemas.