Use attributes for sticky bucketing

[jordangarcia]

Use attributes for sticky bucketing

This PR allows customers to pass in $opt_experiment_bucket_map to attributes. This will take priority over userProfileService.lookup when making sticky bucketing decisions.

Example

// without userProfileService
const client = optimizelySDK.createInstance({
  datafile,
});

const attributes = {
  $opt_experiment_bucket_map: {
    "1": { // abtest experiment_id
      variation_id: "11" // abtest variation1 variation_id
    },
    "2": { // feature test experiment_id
      variation_id: "21" // feature test variation where feature is enabled
    }
  }
}

const variation = client.activate('abtest', 'user1', attributes)
// result will always be variation_id = "11"
const isEnabled = client.isFeatureEnabled('featuretest', 'user1', attributes)
// result will always be true

This allows sticky bucketing to be achieved without using the userProfileService and allows sticky bucketing to be done synchronously. However most implementations the lookup for attributes will be done synchronously and will still use userProfileService.save to persist sticky bucketing information.

Example

const fs = require('fs');
const datafile = require('./datafile.json')
const optimizely = require('./packages/optimizely-sdk/lib/index.node.js')

const EXPERIMENT_KEY = 'local_abtest'
const userProfileService ={
  lookup: function(userId) {
    return null
  },
  save: function(res) {
    let profiles = {}
    try {
      profiles = JSON.parse(fs.readFileSync('userProfiles.json'))
    } catch(e) {}

    let toWrite = {}
    if (profiles[res.user_id]) {
      // merge experiment_bucket_map -> dont override since there could be other sticky bucketing info
      toWrite = profiles[res.user_id]
      const existingBucketMap = toWrite.experiment_bucket_map
      toWrite.experiment_bucket_map = {
        ...existingBucketMap,
        ...res.experiment_bucket_map,
      }
    } else {
      toWrite = res;
    }

    console.log('writing', toWrite);
    profiles[res.user_id] = toWrite
    fs.writeFileSync('userProfiles.json', JSON.stringify(profiles, null, '  '), 'utf8')
  }
}
const client = optimizely.createInstance({
  datafile,
  userProfileService,
});


/**
 * Async example
 */
for (let i = 0; i < 10; i++) {
  // do some async lookup before activating optimizely
  fs.readFile('./userProfiles.json', (err, contents) => {
    if (err) {
      return;
    }

    const userId = `user-${i}`
    const profiles = JSON.parse(contents)
    const userProfile = profiles[userId] || {}
    const attributes = {
      $opt_experiment_bucket_map: userProfile.experiment_bucket_map
    }

    var res1 = client.isFeatureEnabled('f1', userId, attributes);
    var res2 = client.activate('local_abtest', userId, attributes);
    console.log(`userId=${userId} -> ${res1} | ${res2}\n`)
  })
}

[coveralls]

Coverage increased (+0.06%) to 97.321% when pulling 1b8c5e8 on jordan/sticky into 8af7eb2 on master.

[mjc1283]

LGTM, only requesting changes for missing var and updating doc comments.

[mjc1283]

Could we move down inside __resolveExperimentBucketMap, or closer to where it's needed?

Leaving it here makes it harder to merge this into the 2.x branch because this change might affect audience evaluation in that branch. I think we want to release this as a 2.x minor version.

[mjc1283]

Need var here.

[jordangarcia]

Wow, seems like lint should catch something like this and error hard.

[mjc1283]

Nit: delete 'respect'?

[mjc1283]

Nit: the title of this test seems like it should be swapped with the title of the following one.

[mjc1283]

Nit: another doc comment changed

[aliabbasrizvi]

@mikeng13 we should have a compat test for this only targeted at JS SDK

[aliabbasrizvi]

nit. Alphabetize.

[jordangarcia]

this is really a nit

[mikeproeng37]

Looks pretty straightforward! Thanks for doing this.

[mikeproeng37]

The description of the function is no longer accurate. Not sure it was very accurate in the first place either because it wasn't really "stored"

[tylerbrandt]

Want to make sure this extension to the API is actually what we want to do (ref: discussion in doc/slack)

Discussed IRL. I still have reservations because of how it makes something complex (distributed syncing) appear simple, but still require the customer to do that legwork, but if this is what we need to do then we need to do it. My understanding is this isn't required in other SDKs since it's much easier to write blocking I/O.