Merge pull request #278 from splitio/flagsets_baseline

Prepare release v1.12.0 with flag sets support for consumer modes

Author: EmilianoSanchez

CHANGES.txt

@@ -1,15 +1,18 @@
-1.11.1 (December XX, 2023)
-  - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed).
+1.12.0 (December 4, 2023)
+ - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages.
+ - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags.
+ - Updated Redis adapter to handle timeouts and queueing of some missing commands: 'hincrby', 'popNRaw', and 'pipeline.exec'.
+ - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed).
 
 1.11.0 (November 3, 2023)
-  - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation):
-    - Added new variations of the get treatment methods to support evaluating flags in given flag set/s.
-      - getTreatmentsByFlagSet and getTreatmentsByFlagSets
-      - getTreatmentsWithConfigByFlagSets and getTreatmentsWithConfigByFlagSets
-    - Added a new optional Split Filter configuration option. This allows the SDK and Split services to only synchronize the flags in the specified flag sets, avoiding unused or unwanted flags from being synced on the SDK instance, bringing all the benefits from a reduced payload.
-      - Note: Only applicable when the SDK is in charge of the rollout data synchronization. When not applicable, the SDK will log a warning on init.
-    - Added `sets` property to the `SplitView` object returned by the `split` and `splits` methods of the SDK manager to expose flag sets on flag views.
-  - Bugfixing - Fixed SDK key validation in NodeJS to ensure the SDK_READY_TIMED_OUT event is emitted when a client-side type SDK key is provided instead of a server-side one (Related to issue https://github.com/splitio/javascript-client/issues/768).
+ - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation):
+   - Added new variations of the get treatment methods to support evaluating flags in given flag set/s.
+     - getTreatmentsByFlagSet and getTreatmentsByFlagSets
+     - getTreatmentsWithConfigByFlagSets and getTreatmentsWithConfigByFlagSets
+   - Added a new optional Split Filter configuration option. This allows the SDK and Split services to only synchronize the flags in the specified flag sets, avoiding unused or unwanted flags from being synced on the SDK instance, bringing all the benefits from a reduced payload.
+     - Note: Only applicable when the SDK is in charge of the rollout data synchronization. When not applicable, the SDK will log a warning on init.
+   - Added `sets` property to the `SplitView` object returned by the `split` and `splits` methods of the SDK manager to expose flag sets on flag views.
+ - Bugfixing - Fixed SDK key validation in NodeJS to ensure the SDK_READY_TIMED_OUT event is emitted when a client-side type SDK key is provided instead of a server-side one (Related to issue https://github.com/splitio/javascript-client/issues/768).
 
 1.10.0 (October 20, 2023)
  - Added `defaultTreatment` property to the `SplitView` object returned by the `split` and `splits` methods of the SDK manager (Related to issue https://github.com/splitio/javascript-commons/issues/225).
@@ -56,7 +59,7 @@
  - Added a new impressions mode for the SDK called NONE, to be used in factory when there is no desire to capture impressions on an SDK factory to feed Split's analytics engine. Running NONE mode, the SDK will only capture unique keys evaluated for a particular feature flag instead of full blown impressions.
  - Updated SDK telemetry to support pluggable storage, partial consumer mode, and synchronizer.
  - Updated storage implementations to improve the performance of feature flag evaluations (i.e., `getTreatment(s)` method calls) when using the default storage in memory.
- - Updated evaluation flow to avoid unnecessarily storage calls when the SDK is not ready.
+ - Updated evaluation flow (i.e., `getTreatment(s)` method calls) to avoid calling the storage for cached feature flags when the SDK is not ready or ready from cache. It applies to all SDK modes.
 
 1.6.1 (July 22, 2022)
  - Updated GoogleAnalyticsToSplit integration to validate `autoRequire` config parameter and avoid some wrong warning logs when mapping GA hit fields to Split event properties.

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@splitsoftware/splitio-commons",
-  "version": "1.11.0",
+  "version": "1.12.0",
   "description": "Split Javascript SDK common components",
   "main": "cjs/index.js",
   "module": "esm/index.js",

package.json

@@ -3,7 +3,7 @@ import { evaluateFeatures, evaluateFeaturesByFlagSets } from '../index';
 import * as LabelsConstants from '../../utils/labels';
 import { loggerMock } from '../../logger/__tests__/sdkLogger.mock';
 import { _Set } from '../../utils/lang/sets';
-import { returnSetsUnion } from '../../utils/lang/sets';
+import { WARN_FLAGSET_WITHOUT_FLAGS } from '../../logger/constants';
 
 const splitsMock = {
   regular: { 'changeNumber': 1487277320548, 'trafficAllocationSeed': 1667452163, 'trafficAllocation': 100, 'trafficTypeName': 'user', 'name': 'always-on', 'seed': 1684183541, 'configurations': {}, 'status': 'ACTIVE', 'killed': false, 'defaultTreatment': 'off', 'conditions': [{ 'conditionType': 'ROLLOUT', 'matcherGroup': { 'combiner': 'AND', 'matchers': [{ 'keySelector': { 'trafficType': 'user', 'attribute': '' }, 'matcherType': 'ALL_KEYS', 'negate': false, 'userDefinedSegmentMatcherData': { 'segmentName': '' }, 'unaryNumericMatcherData': { 'dataType': '', 'value': 0 }, 'whitelistMatcherData': { 'whitelist': null }, 'betweenMatcherData': { 'dataType': '', 'start': 0, 'end': 0 } }] }, 'partitions': [{ 'treatment': 'on', 'size': 100 }, { 'treatment': 'off', 'size': 0 }], 'label': 'in segment all' }] },
@@ -38,14 +38,7 @@ const mockStorage = {
       return splits;
     },
     getNamesByFlagSets(flagSets) {
-      let toReturn = new _Set([]);
-      flagSets.forEach(flagset => {
-        const featureFlagNames = flagSetsMock[flagset];
-        if (featureFlagNames) {
-          toReturn = returnSetsUnion(toReturn, featureFlagNames);
-        }
-      });
-      return toReturn;
+      return flagSets.map(flagset => flagSetsMock[flagset] || new _Set());
     }
   }
 };
@@ -123,7 +116,7 @@ test('EVALUATOR - Multiple evaluations at once / should return right labels, tre
 
 });
 
-test('EVALUATOR - Multiple evaluations at once by flag sets / should return right labels, treatments and configs if storage returns without errors.', async function () {
+describe('EVALUATOR - Multiple evaluations at once by flag sets', () => {
 
   const expectedOutput = {
     config: {
@@ -135,44 +128,76 @@ test('EVALUATOR - Multiple evaluations at once by flag sets / should return righ
     },
   };
 
-  const getResultsByFlagsets = (flagSets: string[]) => {
+  const getResultsByFlagsets = (flagSets: string[], storage = mockStorage) => {
     return evaluateFeaturesByFlagSets(
       loggerMock,
       'fake-key',
       flagSets,
       null,
-      mockStorage,
+      storage,
+      'method-name'
     );
   };
 
-
-
-  let multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config', 'arch_and_killed']);
-
-  // assert evaluationWithConfig
-  expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); // If the split is retrieved successfully we should get the right evaluation result, label and config.
-  // @todo assert flag set not found - for input validations
-
-  // assert regular
-  expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); // If the split is retrieved successfully we should get the right evaluation result, label and config. If Split has no config it should have config equal null.
-  // assert killed
-  expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual({ ...expectedOutput['config'], treatment: 'off', config: null, label: LabelsConstants.SPLIT_KILLED });
-  // 'If the split is retrieved but is killed, we should get the right evaluation result, label and config.
-
-  // assert archived
-  expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual({ ...expectedOutput['config'], treatment: 'control', label: LabelsConstants.SPLIT_ARCHIVED, config: null });
-  // If the split is retrieved but is archived, we should get the right evaluation result, label and config.
-
-  // assert not_existent_split not in evaluation if it is not related to defined flag sets
-  expect(multipleEvaluationAtOnceByFlagSets['not_existent_split']).toEqual(undefined);
-
-  multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets([]);
-  expect(multipleEvaluationAtOnceByFlagSets).toEqual({});
-
-  multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config']);
-  expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']);
-  expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null });
-  expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual(undefined);
-  expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual(undefined);
-
+  test('should return right labels, treatments and configs if storage returns without errors', async () => {
+
+    let multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config', 'arch_and_killed']);
+
+    // assert evaluationWithConfig
+    expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); // If the split is retrieved successfully we should get the right evaluation result, label and config.
+    // @todo assert flag set not found - for input validations
+
+    // assert regular
+    expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); // If the split is retrieved successfully we should get the right evaluation result, label and config. If Split has no config it should have config equal null.
+    // assert killed
+    expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual({ ...expectedOutput['config'], treatment: 'off', config: null, label: LabelsConstants.SPLIT_KILLED });
+    // 'If the split is retrieved but is killed, we should get the right evaluation result, label and config.
+
+    // assert archived
+    expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual({ ...expectedOutput['config'], treatment: 'control', label: LabelsConstants.SPLIT_ARCHIVED, config: null });
+    // If the split is retrieved but is archived, we should get the right evaluation result, label and config.
+
+    // assert not_existent_split not in evaluation if it is not related to defined flag sets
+    expect(multipleEvaluationAtOnceByFlagSets['not_existent_split']).toEqual(undefined);
+
+    multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets([]);
+    expect(multipleEvaluationAtOnceByFlagSets).toEqual({});
+
+    multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config']);
+    expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']);
+    expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null });
+    expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual(undefined);
+    expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual(undefined);
+  });
+
+  test('should log a warning if evaluating with flag sets that doesn\'t contain cached feature flags', async () => {
+    const getSplitsSpy = jest.spyOn(mockStorage.splits, 'getSplits');
+
+    // No flag set contains cached feature flags -> getSplits method is not called
+    expect(getResultsByFlagsets(['inexistent_set1', 'inexistent_set2'])).toEqual({});
+    expect(getSplitsSpy).not.toHaveBeenCalled();
+    expect(loggerMock.warn.mock.calls).toEqual([
+      [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set1']],
+      [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set2']],
+    ]);
+
+    // One flag set contains cached feature flags -> getSplits method is called
+    expect(getResultsByFlagsets(['inexistent_set3', 'reg_and_config'])).toEqual(getResultsByFlagsets(['reg_and_config']));
+    expect(getSplitsSpy).toHaveBeenLastCalledWith(['regular', 'config']);
+    expect(loggerMock.warn).toHaveBeenLastCalledWith(WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set3']);
+
+    getSplitsSpy.mockRestore();
+    loggerMock.warn.mockClear();
+
+    // Should support async storage too
+    expect(await getResultsByFlagsets(['inexistent_set1', 'inexistent_set2'], {
+      splits: {
+        getNamesByFlagSets(flagSets) { return Promise.resolve(flagSets.map(flagset => flagSetsMock[flagset] || new _Set())); }
+      }
+    })).toEqual({});
+    expect(loggerMock.warn.mock.calls).toEqual([
+      [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set1']],
+      [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set2']],
+    ]);
+  });
 });

src/evaluator/__tests__/evaluate-features.spec.ts

@@ -7,7 +7,8 @@ import { IStorageAsync, IStorageSync } from '../storages/types';
 import { IEvaluationResult } from './types';
 import { SplitIO } from '../types';
 import { ILogger } from '../logger/types';
-import { ISet, setToArray } from '../utils/lang/sets';
+import { ISet, setToArray, returnSetsUnion, _Set } from '../utils/lang/sets';
+import { WARN_FLAGSET_WITHOUT_FLAGS } from '../logger/constants';
 
 const treatmentException = {
   treatment: CONTROL,
@@ -94,8 +95,27 @@ export function evaluateFeaturesByFlagSets(
   flagSets: string[],
   attributes: SplitIO.Attributes | undefined,
   storage: IStorageSync | IStorageAsync,
+  method: string,
 ): MaybeThenable<Record<string, IEvaluationResult>> {
-  let storedFlagNames: MaybeThenable<ISet<string>>;
+  let storedFlagNames: MaybeThenable<ISet<string>[]>;
+
+  function evaluate(
+    featureFlagsByFlagSets: ISet<string>[],
+  ) {
+    let featureFlags = new _Set();
+    for (let i = 0; i < flagSets.length; i++) {
+      const featureFlagByFlagSet = featureFlagsByFlagSets[i];
+      if (featureFlagByFlagSet.size) {
+        featureFlags = returnSetsUnion(featureFlags, featureFlagByFlagSet);
+      } else {
+        log.warn(WARN_FLAGSET_WITHOUT_FLAGS, [method, flagSets[i]]);
+      }
+    }
+
+    return featureFlags.size ?
+      evaluateFeatures(log, key, setToArray(featureFlags), attributes, storage) :
+      {};
+  }
 
   // get features by flag sets
   try {
@@ -107,11 +127,11 @@ export function evaluateFeaturesByFlagSets(
 
   // evaluate related features
   return thenable(storedFlagNames) ?
-    storedFlagNames.then((splitNames) => evaluateFeatures(log, key, setToArray(splitNames), attributes, storage))
+    storedFlagNames.then((storedFlagNames) => evaluate(storedFlagNames))
       .catch(() => {
         return {};
       }) :
-    evaluateFeatures(log, key, setToArray(storedFlagNames), attributes, storage);
+    evaluate(storedFlagNames);
 }
 
 function getEvaluation(

src/evaluator/index.ts

@@ -91,6 +91,7 @@ export const WARN_NOT_EXISTENT_SPLIT = 215;
 export const WARN_LOWERCASE_TRAFFIC_TYPE = 216;
 export const WARN_NOT_EXISTENT_TT = 217;
 export const WARN_INTEGRATION_INVALID = 218;
+export const WARN_SPLITS_FILTER_IGNORED = 219;
 export const WARN_SPLITS_FILTER_INVALID = 220;
 export const WARN_SPLITS_FILTER_EMPTY = 221;
 export const WARN_SDK_KEY = 222;
@@ -99,6 +100,7 @@ export const STREAMING_PARSING_SPLIT_UPDATE = 224;
 export const WARN_SPLITS_FILTER_INVALID_SET = 225;
 export const WARN_SPLITS_FILTER_LOWERCASE_SET = 226;
 export const WARN_FLAGSET_NOT_CONFIGURED = 227;
+export const WARN_FLAGSET_WITHOUT_FLAGS = 228;
 
 export const ERROR_ENGINE_COMBINER_IFELSEIF = 300;
 export const ERROR_LOGLEVEL_INVALID = 301;

src/logger/constants.ts

@@ -24,15 +24,17 @@ export const codesWarn: [number, string][] = codesError.concat([
   [c.WARN_NOT_EXISTENT_SPLIT, '%s: feature flag "%s" does not exist in this environment. Please double check what feature flags exist in the Split user interface.'],
   [c.WARN_LOWERCASE_TRAFFIC_TYPE, '%s: traffic_type_name should be all lowercase - converting string to lowercase.'],
   [c.WARN_NOT_EXISTENT_TT, '%s: traffic type "%s" does not have any corresponding feature flag in this environment, make sure you\'re tracking your events to a valid traffic type defined in the Split user interface.'],
-  [c.WARN_FLAGSET_NOT_CONFIGURED, '%s: : you passed %s wich is not part of the configured FlagSetsFilter, ignoring Flag Set.'],
+  [c.WARN_FLAGSET_NOT_CONFIGURED, '%s: you passed %s which is not part of the configured FlagSetsFilter, ignoring Flag Set.'],
   // initialization / settings validation
-  [c.WARN_INTEGRATION_INVALID, c.LOG_PREFIX_SETTINGS+': %s integration item(s) at settings is invalid. %s'],
-  [c.WARN_SPLITS_FILTER_INVALID, c.LOG_PREFIX_SETTINGS+': feature flag filter at position %s is invalid. It must be an object with a valid filter type ("bySet", "byName" or "byPrefix") and a list of "values".'],
-  [c.WARN_SPLITS_FILTER_EMPTY, c.LOG_PREFIX_SETTINGS+': feature flag filter configuration must be a non-empty array of filter objects.'],
-  [c.WARN_SDK_KEY, c.LOG_PREFIX_SETTINGS+': You already have %s. We recommend keeping only one instance of the factory at all times (Singleton pattern) and reusing it throughout your application'],
+  [c.WARN_INTEGRATION_INVALID, c.LOG_PREFIX_SETTINGS + ': %s integration item(s) at settings is invalid. %s'],
+  [c.WARN_SPLITS_FILTER_IGNORED, c.LOG_PREFIX_SETTINGS + ': feature flag filters are not applicable for Consumer modes where the SDK does not keep rollout data in sync. Filters were discarded'],
+  [c.WARN_SPLITS_FILTER_INVALID, c.LOG_PREFIX_SETTINGS + ': feature flag filter at position %s is invalid. It must be an object with a valid filter type ("bySet", "byName" or "byPrefix") and a list of "values".'],
+  [c.WARN_SPLITS_FILTER_EMPTY, c.LOG_PREFIX_SETTINGS + ': feature flag filter configuration must be a non-empty array of filter objects.'],
+  [c.WARN_SDK_KEY, c.LOG_PREFIX_SETTINGS + ': You already have %s. We recommend keeping only one instance of the factory at all times (Singleton pattern) and reusing it throughout your application'],
 
   [c.STREAMING_PARSING_MY_SEGMENTS_UPDATE_V2, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching MySegments due to an error processing %s notification: %s'],
   [c.STREAMING_PARSING_SPLIT_UPDATE, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching SplitChanges due to an error processing SPLIT_UPDATE notification: %s'],
-  [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS+': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'],
-  [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS+': flag set %s should be all lowercase - converting string to lowercase.'],
+  [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS + ': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'],
+  [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS + ': flag set %s should be all lowercase - converting string to lowercase.'],
+  [c.WARN_FLAGSET_WITHOUT_FLAGS, '%s: you passed %s flag set that does not contain cached feature flag names. Please double check what flag sets are in use in the Split user interface.'],
 ]);