Skip to content
This repository was archived by the owner on Feb 24, 2022. It is now read-only.

Update to track latest nimbus schema#30

Merged
rfk merged 1 commit intomainfrom
latest-nimbus-schema
Oct 8, 2020
Merged

Update to track latest nimbus schema#30
rfk merged 1 commit intomainfrom
latest-nimbus-schema

Conversation

@rfk
Copy link
Copy Markdown
Contributor

@rfk rfk commented Sep 29, 2020

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

Comment thread nimbus/src/evaluator.rs
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();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema has been flattened, there is no longer a separate arguments thing.

Comment thread nimbus/src/http_client.rs Outdated
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(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the example mobile experiment added in mozilla/nimbus-shared#123

@rfk rfk force-pushed the latest-nimbus-schema branch from 9a81611 to 1dcdea9 Compare September 29, 2020 09:37
@piatra
Copy link
Copy Markdown
Contributor

piatra commented Oct 2, 2020

We don't do client side validation against the schema. This can land.
There's a parallel pull request in mozilla/messaging-system-inflight-assets#157 where we deploy messages from (hand written and linted/tested against the schema) until the Experiments platform can do that for us.

Copy link
Copy Markdown
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray for flattening the schema!

@rfk rfk force-pushed the latest-nimbus-schema branch from 1dcdea9 to 61812fe Compare October 7, 2020 02:35
@rfk
Copy link
Copy Markdown
Contributor Author

rfk commented Oct 7, 2020

@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?

@rfk rfk force-pushed the latest-nimbus-schema branch from 61812fe to ca54a79 Compare October 7, 2020 03:50
@rfk rfk marked this pull request as ready for review October 7, 2020 03:50
@mhammond
Copy link
Copy Markdown
Member

mhammond commented Oct 7, 2020

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.

@travis79
Copy link
Copy Markdown
Member

travis79 commented Oct 7, 2020

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.

@rfk rfk mentioned this pull request Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants