Zero sampleRate set recipes allowed to be assigned

[dapperdrop]

Setting an individual recipe's sampleRate to 0 (let's call this recipe "C"), whilst leaving other recipe's sampleRates untouched will still result in recipe "C" being allowed to be assigned.

To reproduce:

1. Setup an example test with 3 recipes, "A", "B" and "C"
2. Set recipe "C"'s sampleRate to 0, whilst leaving it untouched in other recipes
3. Launch the test
4. Load the target page and clear cookies until recipe "C" is assigned.

Expected result: Recipe "C" should not be allowed to assigned given it's sampleRate.

Actual result: Recipe "C" can be assigned by chance.

[dapperdrop]

Hi @allmywant - would you be able to take a look at the bug identified here?

[kingo55]

Yes, and it gets assigned according to a 33/33/33 split.

Should we just alert users when other recipes' sample rates are unset? E.g. validation fails when sampleRate is only set on one recipe?

[allmywant]

@dapperdrop @kingo55 Currently, The sampleRate on recipes only works when all recipes of the same test have sampleRate defined. For example: recipe A: 0.5 recipe B: 0.2 recipe C: 0.3

If one recipe has no sampleRate defined then the sampleRate on other recipes won't work. To let it work we can try recipe A: 0.5 recipe B: 0.5 recipe C: 0

Should we just alert users when other recipes' sample rates are unset? E.g. validation fails when sampleRate is only set on one recipe?

I think we can add this validation.

[kingo55]

Seems like there are probably 2 places it can be addressed:

1. Parsing the test objects through the JSON schema (wave.schema.json)
2. Directly in the Mojito lib test object parsing
3. Alternatively, I've wondered whether we might need to implement this logic in the Gulp scripts.

At a minimum, I think we should do #1. Number #2 is a little more fool proof, but number 3 might need some rethinking.

On Mon, 15 Mar 2021 at 20:21, David Lee @.***> wrote:

@dapperdrop https://github.com/dapperdrop @kingo55 https://github.com/kingo55 Currently, The sampleRate on recipes only works when all recipes of the same test have sampleRate defined. For example: recipe A: 0.5 recipe B: 0.2 recipe C: 0.3

If one recipe has no sampleRate defined then the sampleRate on other recipes won't work. To let it work we can try recipe A: 0.5 recipe B: 0.5 recipe C: 0

Should we just alert users when other recipes' sample rates are unset? E.g. validation fails when sampleRate is only set on one recipe?

I think we can add this validation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mint-metrics/mojito-js-delivery/issues/91#issuecomment-799257613, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASAQLBJXZH7L7EMQ2Y7CZ3TDXGQJANCNFSM4ZFZKFCA .

[dapperdrop]

@allmywant @kingo55

Agree with you guys. I think having redundant validations is a good idea, i.e. suggestions 1 and 2.

[kingo55]

@dapperdrop @kingo55 Currently, The sampleRate on recipes only works when all recipes of the same test have sampleRate defined. For example: recipe A: 0.5 recipe B: 0.2 recipe C: 0.3

If one recipe has no sampleRate defined then the sampleRate on other recipes won't work. To let it work we can try recipe A: 0.5 recipe B: 0.5 recipe C: 0

Should we just alert users when other recipes' sample rates are unset? E.g. validation fails when sampleRate is only set on one recipe?

I think we can add this validation.

@allmywant - basically, we need to decide whether to?:

1. Fail the test and stop it from activating
2. Attempt to correct the test

I think option 1 is probably the safest. It might also introduce less code into the library.

What do you guys think?

[dapperdrop]

Option 1 is sensible. Allow the user to correct.

[allmywant]

@kingo55 Option 1 is better.

[kingo55]

@dapperdrop @allmywant - thanks guys. Would you mind taking this change on please, David?

[allmywant]

@kingo55 @dapperdrop I have sent a pull request.

[dapperdrop]

Addressed in PRs #92 and #93

Thank you @kingo55 @allmywant