Conversation
| pub fn filter_enrolled(id: &Uuid, experiments: &[Experiment]) -> Result<Vec<EnrolledExperiment>> { | ||
| let mut res = Vec::with_capacity(experiments.len()); | ||
| for exp in experiments { | ||
| let bucket_config = exp.arguments.bucket_config.clone(); |
There was a problem hiding this comment.
The schema has been flattened, there is no longer a separate arguments thing.
| Branch { slug: "treatment-variation-b".to_string(), ratio: 1, feature: FeatureConfig { feature_id: Group::Cfr, enabled: true, value: None } } | ||
| ] | ||
| }, | ||
| slug: "mobile-a-a-example".to_string(), |
There was a problem hiding this comment.
This matches the example mobile experiment added in mozilla/nimbus-shared#123
9a81611 to
1dcdea9
Compare
|
We don't do client side validation against the schema. This can land. |
travis79
left a comment
There was a problem hiding this comment.
Hooray for flattening the schema!
1dcdea9 to
61812fe
Compare
|
@mhammond flagging this for your context based on conversation in the meeting today. This PR is up-to-date with the latest schema definitions from https://github.com/mozilla/nimbus-shared, although there are some pending discussions around the details (e.g. whether to adding an explicit version number). The data currently in the "messaging-experiments" bucket on remote settings is not compatible with this schema. And I don't believe we have any data deployed that is compatible. There is however a mobile-a-a.json sample schema that we can use, e.g. by publishing to the dev kinto instance. We also have a new "nimbus-mobile-experiments" bucket in remote-settings, which IIUC should replace "messaging-experiments" as our default bucket. It's currently empty, but I've asked in slack about how we might get the sample schema published there. I'm taking this PR out of draft, but it's not clear to me if there's value in merging it until we have an updated sample schema somewhere we can query it. Thoughts? |
61812fe to
ca54a79
Compare
|
So IIUC, there's currently no data that conforms to what's currently in our repo, and there never will be. There's also no data that conforms to what we believe is correct (ie, what's done in this PR), but we expect that this will exist at some point in the future. If that's correct, I see no reason not to merge it - it will at least help everyone work out what's wrong with either the future data or the expectations codified here. |
|
I agree with Mark. If this is the current schema definition, even if this is the only thing implementing it right now, we can always iterate on this again as the schema evolves. |
be5fc2e to
f8f10e9
Compare
The experiment schema defined over in nimbus-shared has had a few modifications recently, which IMHO make it substantially better for our purposes. I was going through the details and figured I might as well update the rust code to track it while I was in there.
This is WIP only because I don't know whether we need to wait for these schema changes to get deployed to a server somewhere, ref https://jira.mozilla.com/browse/SYNC-1794