Refactor strategies and impressionsTracker

Author: EmilianoSanchez

src/sdkFactory/index.ts

@@ -13,7 +13,7 @@ import { strategyDebugFactory } from '../trackers/strategy/strategyDebug';
 import { strategyOptimizedFactory } from '../trackers/strategy/strategyOptimized';
 import { strategyNoneFactory } from '../trackers/strategy/strategyNone';
 import { uniqueKeysTrackerFactory } from '../trackers/uniqueKeysTracker';
-import { NONE, OPTIMIZED } from '../utils/constants';
+import { DEBUG, OPTIMIZED } from '../utils/constants';
 
 /**
  * Modular SDK factory
@@ -61,19 +61,14 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ISDK | SplitIO.IA
   const observer = impressionsObserverFactory();
   const uniqueKeysTracker = uniqueKeysTrackerFactory(log, storage.uniqueKeys, filterAdapterFactory && filterAdapterFactory());
 
-  let strategy;
-  switch (impressionsMode) {
-    case OPTIMIZED:
-      strategy = strategyOptimizedFactory(observer, storage.impressionCounts);
-      break;
-    case NONE:
-      strategy = strategyNoneFactory(storage.impressionCounts, uniqueKeysTracker);
-      break;
-    default:
-      strategy = strategyDebugFactory(observer);
-  }
+  const noneStrategy = strategyNoneFactory(storage.impressionCounts, uniqueKeysTracker);
+  const strategy = impressionsMode === OPTIMIZED ?
+    strategyOptimizedFactory(observer, storage.impressionCounts) :
+    impressionsMode === DEBUG ?
+      strategyDebugFactory(observer) :
+      noneStrategy;
 
-  const impressionsTracker = impressionsTrackerFactory(settings, storage.impressions, strategy, whenInit, integrationsManager, storage.telemetry);
+  const impressionsTracker = impressionsTrackerFactory(settings, storage.impressions, noneStrategy, strategy, whenInit, integrationsManager, storage.telemetry);
   const eventTracker = eventTrackerFactory(settings, storage.events, whenInit, integrationsManager, storage.telemetry);
 
   // splitApi is used by SyncManager and Browser signal listener

src/trackers/__tests__/impressionsTracker.spec.ts

@@ -36,6 +36,11 @@ const fakeSettingsWithListener = {
 };
 const fakeWhenInit = (cb: () => void) => cb();
 
+// @TODO validate logic with `impression.track` false
+const fakeNoneStrategy = {
+  process: jest.fn(() => false)
+};
+
 /* Tests */
 
 describe('Impressions Tracker', () => {
@@ -51,12 +56,12 @@ describe('Impressions Tracker', () => {
   test('Tracker API', () => {
     expect(typeof impressionsTrackerFactory).toBe('function'); // The module should return a function which acts as a factory.
 
-    const instance = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, strategy, fakeWhenInit);
+    const instance = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy,strategy, fakeWhenInit);
     expect(typeof instance.track).toBe('function'); // The instance should implement the track method which will actually track queued impressions.
   });
 
   test('Should be able to track impressions (in DEBUG mode without Previous Time).', () => {
-    const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, strategy, fakeWhenInit);
+    const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategy, fakeWhenInit);
 
     const imp1 = {
       feature: '10',
@@ -76,7 +81,7 @@ describe('Impressions Tracker', () => {
   });
 
   test('Tracked impressions should be sent to impression listener and integration manager when we invoke .track()', (done) => {
-    const tracker = impressionsTrackerFactory(fakeSettingsWithListener, fakeImpressionsCache, strategy, fakeWhenInit, fakeIntegrationsManager);
+    const tracker = impressionsTrackerFactory(fakeSettingsWithListener, fakeImpressionsCache, fakeNoneStrategy, strategy, fakeWhenInit, fakeIntegrationsManager);
 
     const fakeImpression = {
       feature: 'impression'
@@ -150,8 +155,8 @@ describe('Impressions Tracker', () => {
     impression3.time = 1234567891;
 
     const trackers = [
-      impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, strategyDebugFactory(impressionObserverSSFactory()), fakeWhenInit, undefined),
-      impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, strategyDebugFactory(impressionObserverCSFactory()), fakeWhenInit, undefined)
+      impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyDebugFactory(impressionObserverSSFactory()), fakeWhenInit, undefined),
+      impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyDebugFactory(impressionObserverCSFactory()), fakeWhenInit, undefined)
     ];
 
     expect(fakeImpressionsCache.track).not.toBeCalled(); // storage method should not be called until impressions are tracked.
@@ -178,7 +183,7 @@ describe('Impressions Tracker', () => {
     impression3.time = Date.now();
 
     const impressionCountsCache = new ImpressionCountsCacheInMemory();
-    const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, strategyOptimizedFactory(impressionObserverCSFactory(), impressionCountsCache), fakeWhenInit, undefined, fakeTelemetryCache as any);
+    const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyOptimizedFactory(impressionObserverCSFactory(), impressionCountsCache), fakeWhenInit, undefined, fakeTelemetryCache as any);
 
     expect(fakeImpressionsCache.track).not.toBeCalled(); // cache method should not be called by just creating a tracker
 
@@ -201,7 +206,7 @@ describe('Impressions Tracker', () => {
   test('Should track or not impressions depending on user consent status', () => {
     const settings = { ...fullSettings };
 
-    const tracker = impressionsTrackerFactory(settings, fakeImpressionsCache, strategy, fakeWhenInit);
+    const tracker = impressionsTrackerFactory(settings, fakeImpressionsCache, fakeNoneStrategy, strategy, fakeWhenInit);
 
     tracker.track([impression]);
     expect(fakeImpressionsCache.track).toBeCalledTimes(1); // impression should be tracked if userConsent is undefined

src/trackers/impressionsTracker.ts

@@ -9,16 +9,11 @@ import SplitIO from '../../types/splitio';
 
 /**
  * Impressions tracker stores impressions in cache and pass them to the listener and integrations manager if provided.
- *
- * @param impressionsCache - cache to save impressions
- * @param metadata - runtime metadata (ip, hostname and version)
- * @param impressionListener - optional impression listener
- * @param integrationsManager - optional integrations manager
- * @param strategy - strategy for impressions tracking.
  */
 export function impressionsTrackerFactory(
   settings: ISettings,
   impressionsCache: IImpressionsCacheBase,
+  noneStrategy: IStrategy,
   strategy: IStrategy,
   whenInit: (cb: () => void) => void,
   integrationsManager?: IImpressionsHandler,
@@ -31,37 +26,41 @@ export function impressionsTrackerFactory(
     track(impressions: SplitIO.ImpressionDTO[], attributes?: SplitIO.Attributes) {
       if (settings.userConsent === CONSENT_DECLINED) return;
 
-      const impressionsCount = impressions.length;
-      const { impressionsToStore, impressionsToListener, deduped } = strategy.process(impressions);
+      const impressionsToStore = impressions.filter((impression) => {
+        return impression.track === false ?
+          noneStrategy.process(impression) :
+          strategy.process(impression);
+      });
 
-      const impressionsToListenerCount = impressionsToListener.length;
+      const impressionsLength = impressions.length;
+      const impressionsToStoreLength = impressionsToStore.length;
 
-      if (impressionsToStore.length > 0) {
+      if (impressionsToStoreLength) {
         const res = impressionsCache.track(impressionsToStore);
 
         // If we're on an async storage, handle error and log it.
         if (thenable(res)) {
           res.then(() => {
-            log.info(IMPRESSIONS_TRACKER_SUCCESS, [impressionsCount]);
+            log.info(IMPRESSIONS_TRACKER_SUCCESS, [impressionsLength]);
           }).catch(err => {
-            log.error(ERROR_IMPRESSIONS_TRACKER, [impressionsCount, err]);
+            log.error(ERROR_IMPRESSIONS_TRACKER, [impressionsLength, err]);
           });
         } else {
           // Record when impressionsCache is sync only (standalone mode)
           // @TODO we are not dropping impressions on full queue yet, so DROPPED stats are not recorded
           if (telemetryCache) {
-            (telemetryCache as ITelemetryCacheSync).recordImpressionStats(QUEUED, impressionsToStore.length);
-            (telemetryCache as ITelemetryCacheSync).recordImpressionStats(DEDUPED, deduped);
+            (telemetryCache as ITelemetryCacheSync).recordImpressionStats(QUEUED, impressionsToStoreLength);
+            (telemetryCache as ITelemetryCacheSync).recordImpressionStats(DEDUPED, impressionsLength - impressionsToStoreLength);
           }
         }
       }
 
       // @TODO next block might be handled by the integration manager. In that case, the metadata object doesn't need to be passed in the constructor
       if (impressionListener || integrationsManager) {
-        for (let i = 0; i < impressionsToListenerCount; i++) {
+        for (let i = 0; i < impressionsLength; i++) {
           const impressionData: SplitIO.ImpressionData = {
             // copy of impression, to avoid unexpected behaviour if modified by integrations or impressionListener
-            impression: objectAssign({}, impressionsToListener[i]),
+            impression: objectAssign({}, impressions[i]),
             attributes,
             ip,
             hostname,

src/trackers/strategy/__tests__/strategyDebug.spec.ts

@@ -3,29 +3,21 @@ import { impressionObserverCSFactory } from '../../impressionObserver/impression
 import { strategyDebugFactory } from '../strategyDebug';
 import { impression1, impression2 } from './testUtils';
 
-test('strategyDebug', () => {
+test.each([
+  impressionObserverSSFactory(),
+  impressionObserverCSFactory()]
+)('strategyDebug', (impressionObserver) => {
 
-  let augmentedImp1 = { ...impression1, pt: undefined };
-  let augmentedImp12 = { ...impression1, pt: impression1.time };
-  let augmentedImp2 = { ...impression2, pt: undefined };
+  const strategyDebug = strategyDebugFactory(impressionObserver);
 
-  let impressions = [impression1, impression2, {...impression1}];
-  let augmentedImpressions = [augmentedImp1, augmentedImp2, augmentedImp12];
+  const impressions = [{ ...impression1 },  { ...impression2 }, { ...impression1 }];
 
-  const strategyDebugSS = strategyDebugFactory(impressionObserverSSFactory());
+  expect(strategyDebug.process(impressions[0])).toBe(true);
+  expect(impressions[0]).toEqual({ ...impression1, pt: undefined });
 
-  let { impressionsToStore, impressionsToListener, deduped } = strategyDebugSS.process(impressions);
-
-  expect(impressionsToStore).toStrictEqual(augmentedImpressions);
-  expect(impressionsToListener).toStrictEqual(augmentedImpressions);
-  expect(deduped).toStrictEqual(0);
-
-  const strategyDebugCS = strategyDebugFactory(impressionObserverCSFactory());
-
-  ({ impressionsToStore, impressionsToListener, deduped } = strategyDebugCS.process(impressions));
-
-  expect(impressionsToStore).toStrictEqual(augmentedImpressions);
-  expect(impressionsToListener).toStrictEqual(augmentedImpressions);
-  expect(deduped).toStrictEqual(0);
+  expect(strategyDebug.process(impressions[1])).toBe(true);
+  expect(impressions[1]).toEqual({ ...impression2, pt: undefined });
 
+  expect(strategyDebug.process(impressions[2])).toBe(true);
+  expect(impressions[2]).toEqual({ ...impression1, pt: impression1.time });
 });

src/trackers/strategy/__tests__/strategyNone.spec.ts

@@ -28,11 +28,9 @@ test('strategyNone - Client side', () => {
 
   const strategyNone = strategyNoneFactory(impressionCountsCache, uniqueKeysTracker);
 
-  const {
-    impressionsToStore: impressionsToStoreCs,
-    impressionsToListener: impressionsToListenerCs,
-    deduped: dedupedCs
-  } = strategyNone.process(impressions);
+  impressions.forEach(impression => {
+    expect(strategyNone.process(impression)).toBe(false);
+  });
 
   expect(uniqueKeysCacheCS.pop()).toStrictEqual({
     keys: [
@@ -52,11 +50,6 @@ test('strategyNone - Client side', () => {
   });
 
   expect(uniqueKeysCacheCS.pop()).toStrictEqual({ keys: [] });
-
-  expect(impressionsToStoreCs).toStrictEqual([]);
-  expect(impressionsToListenerCs).toStrictEqual(impressions);
-  expect(dedupedCs).toStrictEqual(0);
-
 });
 
 test('strategyNone - Server side', () => {
@@ -67,11 +60,9 @@ test('strategyNone - Server side', () => {
 
   const strategyNone = strategyNoneFactory(impressionCountsCache, uniqueKeysTracker);
 
-  const {
-    impressionsToStore: impressionsToStoreSs,
-    impressionsToListener: impressionsToListenerSs,
-    deduped: dedupedSs
-  } = strategyNone.process(impressions);
+  impressions.forEach(impression => {
+    expect(strategyNone.process(impression)).toBe(false);
+  });
 
   expect(uniqueKeysCache.pop()).toStrictEqual({
     keys: [
@@ -90,9 +81,4 @@ test('strategyNone - Server side', () => {
     ]
   });
   expect(uniqueKeysCache.pop()).toStrictEqual({ keys: [] });
-
-  expect(impressionsToStoreSs).toStrictEqual([]);
-  expect(impressionsToListenerSs).toStrictEqual(impressions);
-  expect(dedupedSs).toStrictEqual(0);
-
 });

src/trackers/strategy/__tests__/strategyOptimized.spec.ts

@@ -4,31 +4,25 @@ import { strategyOptimizedFactory } from '../strategyOptimized';
 import { ImpressionCountsCacheInMemory } from '../../../storages/inMemory/ImpressionCountsCacheInMemory';
 import { impression1, impression2 } from './testUtils';
 
-test('strategyOptimized', () => {
-
-  let augmentedImp1 = { ...impression1, pt: undefined };
-  let augmentedImp12 = { ...impression1, pt: impression1.time };
-  let augmentedImp13 = { ...impression1, pt: impression1.time };
-  let augmentedImp2 = { ...impression2, pt: undefined };
+test.each([
+  impressionObserverSSFactory(),
+  impressionObserverCSFactory()
+])('strategyOptimized', () => {
 
   const impressionCountsCache = new ImpressionCountsCacheInMemory();
-  const impressions = [impression1, impression2, {...impression1}, {...impression1}];
-  const augmentedImpressions = [augmentedImp1, augmentedImp2, augmentedImp12, augmentedImp13];
-
   const strategyOptimizedSS = strategyOptimizedFactory(impressionObserverSSFactory(), impressionCountsCache);
 
-  let { impressionsToStore, impressionsToListener, deduped } = strategyOptimizedSS.process(impressions);
-
-  expect(impressionsToStore).toStrictEqual([augmentedImp1, augmentedImp2]);
-  expect(impressionsToListener).toStrictEqual(augmentedImpressions);
-  expect(deduped).toStrictEqual(2);
+  const impressions = [{...impression1}, {...impression2}, {...impression1}, {...impression1}];
 
-  const strategyOptimizedCS = strategyOptimizedFactory(impressionObserverCSFactory(), impressionCountsCache);
+  expect(strategyOptimizedSS.process(impressions[0])).toBe(true);
+  expect(impressions[0]).toEqual({ ...impression1, pt: undefined });
 
-  ({ impressionsToStore, impressionsToListener, deduped } = strategyOptimizedCS.process(impressions));
+  expect(strategyOptimizedSS.process(impressions[1])).toBe(true);
+  expect(impressions[1]).toEqual({ ...impression2, pt: undefined });
 
-  expect(impressionsToStore).toStrictEqual([augmentedImp1, augmentedImp2]);
-  expect(impressionsToListener).toStrictEqual(augmentedImpressions);
-  expect(deduped).toStrictEqual(2);
+  expect(strategyOptimizedSS.process(impressions[2])).toBe(false);
+  expect(impressions[2]).toEqual({ ...impression1, pt: impression1.time });
 
+  expect(strategyOptimizedSS.process(impressions[3])).toBe(false);
+  expect(impressions[3]).toEqual({ ...impression1, pt: impression1.time });
 });

src/trackers/strategy/strategyDebug.ts

@@ -6,23 +6,16 @@ import { IStrategy } from '../types';
  * Debug strategy for impressions tracker. Wraps impressions to store and adds previousTime if it corresponds
  *
  * @param impressionsObserver - impression observer. Previous time (pt property) is included in impression instances
- * @returns IStrategyResult
+ * @returns Debug strategy
  */
 export function strategyDebugFactory(
   impressionsObserver: IImpressionObserver
 ): IStrategy {
 
   return {
-    process(impressions: SplitIO.ImpressionDTO[]) {
-      impressions.forEach((impression) => {
-        // Adds previous time if it is enabled
-        impression.pt = impressionsObserver.testAndSet(impression);
-      });
-      return {
-        impressionsToStore: impressions,
-        impressionsToListener: impressions,
-        deduped: 0
-      };
+    process(impression: SplitIO.ImpressionDTO) {
+      impression.pt = impressionsObserver.testAndSet(impression);
+      return true;
     }
   };
 }

src/trackers/strategy/strategyNone.ts

@@ -5,30 +5,24 @@ import { IStrategy, IUniqueKeysTracker } from '../types';
 /**
  * None strategy for impressions tracker.
  *
- * @param impressionsCounter - cache to save impressions count. impressions will be deduped (OPTIMIZED mode)
+ * @param impressionCounts - cache to save impressions count. impressions will be deduped (OPTIMIZED mode)
  * @param uniqueKeysTracker - unique keys tracker in charge of tracking the unique keys per split.
- * @returns IStrategyResult
+ * @returns None strategy
  */
 export function strategyNoneFactory(
-  impressionsCounter: IImpressionCountsCacheBase,
+  impressionCounts: IImpressionCountsCacheBase,
   uniqueKeysTracker: IUniqueKeysTracker
 ): IStrategy {
 
   return {
-    process(impressions: SplitIO.ImpressionDTO[]) {
-      impressions.forEach((impression) => {
-        const now = Date.now();
-        // Increments impression counter per featureName
-        impressionsCounter.track(impression.feature, now, 1);
-        // Keep track by unique key
-        uniqueKeysTracker.track(impression.keyName, impression.feature);
-      });
-
-      return {
-        impressionsToStore: [],
-        impressionsToListener: impressions,
-        deduped: 0
-      };
+    process(impression: SplitIO.ImpressionDTO) {
+      const now = Date.now();
+      // Increments impression counter per featureName
+      impressionCounts.track(impression.feature, now, 1);
+      // Keep track by unique key
+      uniqueKeysTracker.track(impression.keyName, impression.feature);
+      // Do not store impressions
+      return false;
     }
   };
 }