Merge branch 'redis_storage_improvements' into sdks-7658

CHANGES.txt

@@ -1,3 +1,8 @@
+1.12.0 (December XX, 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 wrap missing commands: 'hincrby', 'popNRaw', 'flushdb' and 'pipeline.exec'.
+
 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.
@@ -53,7 +58,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.json

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

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

@@ -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/index.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/logger/constants.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/messages/warn.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.'],
 ]);

src/readiness/__tests__/sdkReadinessManager.spec.ts

@@ -49,7 +49,7 @@ describe('SDK Readiness Manager - Event emitter', () => {
       expect(sdkStatus[propName]).toBeTruthy(); // The sdkStatus exposes all minimal EventEmitter functionality.
     });
 
-    expect(typeof sdkStatus['ready']).toBe('function'); // The sdkStatus exposes a .ready() function.
+    expect(typeof sdkStatus.ready).toBe('function'); // The sdkStatus exposes a .ready() function.
 
     expect(typeof sdkStatus.Event).toBe('object'); // It also exposes the Event map,
     expect(sdkStatus.Event.SDK_READY).toBe(SDK_READY); // which contains the constants for the events, for backwards compatibility.

src/sdkClient/client.ts

@@ -1,8 +1,8 @@
 import { evaluateFeature, evaluateFeatures, evaluateFeaturesByFlagSets } from '../evaluator';
 import { thenable } from '../utils/promise/thenable';
 import { getMatching, getBucketing } from '../utils/key';
-import { validateSplitExistance } from '../utils/inputValidation/splitExistance';
-import { validateTrafficTypeExistance } from '../utils/inputValidation/trafficTypeExistance';
+import { validateSplitExistence } from '../utils/inputValidation/splitExistence';
+import { validateTrafficTypeExistence } from '../utils/inputValidation/trafficTypeExistence';
 import { SDK_NOT_READY } from '../utils/labels';
 import { CONTROL, TREATMENT, TREATMENTS, TREATMENT_WITH_CONFIG, TREATMENTS_WITH_CONFIG, TRACK, TREATMENTS_WITH_CONFIG_BY_FLAGSETS, TREATMENTS_BY_FLAGSETS, TREATMENTS_BY_FLAGSET, TREATMENTS_WITH_CONFIG_BY_FLAGSET } from '../utils/constants';
 import { IEvaluationResult } from '../evaluator/types';
@@ -99,7 +99,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl
     };
 
     const evaluations = readinessManager.isReady() || readinessManager.isReadyFromCache() ?
-      evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage) :
+      evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage, method) :
       isStorageSync(settings) ? {} : Promise.resolve({}); // Promisify if async
 
     return thenable(evaluations) ? evaluations.then((res) => wrapUp(res)) : wrapUp(evaluations);
@@ -133,7 +133,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl
     const { treatment, label, changeNumber, config = null } = evaluation;
     log.info(IMPRESSION, [featureFlagName, matchingKey, treatment, label]);
 
-    if (validateSplitExistance(log, readinessManager, featureFlagName, label, invokingMethodName)) {
+    if (validateSplitExistence(log, readinessManager, featureFlagName, label, invokingMethodName)) {
       log.info(IMPRESSION_QUEUEING);
       queue.push({
         feature: featureFlagName,
@@ -171,7 +171,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl
     };
 
     // This may be async but we only warn, we don't actually care if it is valid or not in terms of queueing the event.
-    validateTrafficTypeExistance(log, readinessManager, storage.splits, mode, trafficTypeName, 'track');
+    validateTrafficTypeExistence(log, readinessManager, storage.splits, mode, trafficTypeName, 'track');
 
     const result = eventTracker.track(eventData, size);