Closed Bug 1572039 Opened 1 year ago Closed 11 months ago

Clients seem to be enrolled into multi-preference experiments, but they are not

Categories

(Firefox :: Normandy Client, defect, P1)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: ppop, Assigned: glasserc)

Details

Attachments

(3 files)

[Affected Platforms]:

  • All Windows
  • All Mac
  • All Linux

[Affected Versions]:

  • Firefox Nightly 70.0a1, Build ID 20190806214332
  • Firefox Beta 69.0b11, Build ID 20190805120428

[Prerequisites]:

[Steps to reproduce]:

  1. Open the browser with the profile from prerequisites.
  2. Navigate to about:studies and observe the page.

[Expected result]:

  • The experiment is displayed on the page as active.

[Actual result]:

  • The experiment is not displayed on the page.

[Notes]:

  • The following browser console logs are displayed:
    1565167192865 app.normandy.preference-experiments DEBUG PreferenceExperiments.has(mythmon-multi-preference-example)
    1565167192867 app.normandy.preference-experiments DEBUG PreferenceExperiments.start(mythmon-multi-preference-example, twos)
  • The following error is displayed after checking the "Show Content Messages" cehckbox:
    Invalid value for preferenceBranchType: undefined
  • On the about:telemetry page, events section, the following timestamp is displayed:
    4250 normandy enrollFailed preference_study mythmon-multi-preference-example {"reason": "invalid-branch"}
  • I have attached a screenshot of the console output:

@Michael, @Ethan Could one of you take a look at this please?

Flags: needinfo?(mcooper)
Flags: needinfo?(eglassercamp)

It looks like preferenceBranchType is required for each preference, even though the schema calls it optional. Can you try to add "preferenceBranchType": "default" to each of the preference sections? It would be a sibling of preferenceValue.

Flags: needinfo?(mcooper)

Ciprian told me on slack that my suggestion about the schema fixed the immediate problem. That means that the real problem here is that either

  1. The schema is wrong (allows a null value, and says there is a default)
  2. or the client is not using the schema correctly (assuming a non-null value when none is guaranteed)

I think we can fix it either by updating the schema to be stricter or the client to be more permissive. I think that making the schema stricter makes sense. Ethan, do you have an opinion?

Hmm. I copied this part of the schema from the single-preference-experiment schema, and the single-preference-experiment schema code that predated me seemed to also require preferenceBranchType -- see for example https://phabricator.services.mozilla.com/D29872. I understood this to mean that the default would be substituted when the data was run through the schema. Looking at JsonSchemaValidator now, I see that isn't true. Other uses of default seem to mostly be safe because the client checks for truthiness, which would include missing values.

I think the more fundamental bug is in the client, and we should fix it there (either by fixing JsonSchemaValidator or working around it). But because we have to support old clients too, we should probably make the schema stricter as well, by adding preferenceBranchType to the list of required fields.

Flags: needinfo?(eglassercamp)

The priority flag is not set for this bug.
:mythmon, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mcooper)
Flags: needinfo?(mcooper)
Priority: -- → P1

Looking at this again, it seems like the behavior I expected for default does not seem to be intended by the spec or its authors. See e.g. https://github.com/json-schema/json-schema/issues/5 -- the default is only meant for documentation and does not play a role in validation. JsonSchemaValidator already makes some decisions about how to manipulate the data that do not conform to the spec (for example, converting numbers to booleans, or supporting types such as URL or URLorEmpty). On the other hand, it doesn't seem obvious to add unconditional support for default. (Do we support it in patternProperties?) So I took the approach of updating the schema to make preferenceBranchType required, and also updating code to use default if none is provided. The first patch is necessary in any case and the second may be considered overkill so maybe we should just stick with the first patch? I'm open to suggestions on this point.

I messed up my build configuration for Gecko so at the moment I'm relying on try to test this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=959b5d28ccaf091c8bd7833c239b6441709c92d8

Pushed by eglassercamp@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9ba0489f0c8
Insist on preferenceBranchType. r=mythmon
https://hg.mozilla.org/integration/autoland/rev/28bdd68aac91
Enforce default preferenceBranchType in code. r=mythmon
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Assignee: nobody → eglassercamp

Does this need uplift to 70?

Flags: needinfo?(eglassercamp)

I don't think it's necessary.

Flags: needinfo?(eglassercamp)

I have verified that this issue is no longer reproducible after adding the "preferenceBranchType": "default" argument in the json blob.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.