Merge pull request #374 from splitio/impressions_toggle_strategy_refactors

[Impressions toggle] Strategy refactors

Author: EmilianoSanchez

src/sdkClient/__tests__/sdkClientMethod.spec.ts

@@ -16,7 +16,8 @@ const paramMocks = [
     signalListener: undefined,
     settings: { mode: CONSUMER_MODE, log: loggerMock, core: { authorizationKey: 'sdk key '} },
     telemetryTracker: telemetryTrackerFactory(),
-    clients: {}
+    clients: {},
+    uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() }
   },
   // SyncManager (i.e., Sync SDK) and Signal listener
   {
@@ -26,7 +27,8 @@ const paramMocks = [
     signalListener: { stop: jest.fn() },
     settings: { mode: STANDALONE_MODE, log: loggerMock, core: { authorizationKey: 'sdk key '} },
     telemetryTracker: telemetryTrackerFactory(),
-    clients: {}
+    clients: {},
+    uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() }
   }
 ];
 
@@ -70,6 +72,7 @@ test.each(paramMocks)('sdkClientMethodFactory', (params, done: any) => {
           client.destroy().then(() => {
             expect(params.sdkReadinessManager.readinessManager.destroy).toBeCalledTimes(1);
             expect(params.storage.destroy).toBeCalledTimes(1);
+            expect(params.uniqueKeysTracker.stop).toBeCalledTimes(1);
 
             if (params.syncManager) {
               expect(params.syncManager.stop).toBeCalledTimes(1);

src/sdkClient/__tests__/sdkClientMethodCS.spec.ts

@@ -46,6 +46,7 @@ const params = {
   settings: settingsWithKey,
   telemetryTracker: telemetryTrackerFactory(),
   clients: {},
+  uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() }
 };
 
 const invalidAttributes = [
@@ -95,6 +96,7 @@ describe('sdkClientMethodCSFactory', () => {
       expect(params.syncManager.stop).toBeCalledTimes(1);
       expect(params.syncManager.flush).toBeCalledTimes(1);
       expect(params.signalListener.stop).toBeCalledTimes(1);
+      expect(params.uniqueKeysTracker.stop).toBeCalledTimes(1);
     });
 
   });

src/sdkClient/sdkClient.ts

@@ -61,7 +61,7 @@ export function sdkClientFactory(params: ISdkFactoryContext, isSharedClient?: bo
           releaseApiKey(settings.core.authorizationKey);
           telemetryTracker.sessionLength();
           signalListener && signalListener.stop();
-          uniqueKeysTracker && uniqueKeysTracker.stop();
+          uniqueKeysTracker.stop();
         }
 
         // Stop background jobs

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
@@ -59,21 +59,16 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ISDK | SplitIO.IA
   const integrationsManager = integrationsManagerFactory && integrationsManagerFactory({ settings, storage, telemetryTracker });
 
   const observer = impressionsObserverFactory();
-  const uniqueKeysTracker = impressionsMode === NONE ? uniqueKeysTrackerFactory(log, storage.uniqueKeys!, filterAdapterFactory && filterAdapterFactory()) : undefined;
-
-  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 uniqueKeysTracker = uniqueKeysTrackerFactory(log, storage.uniqueKeys, filterAdapterFactory && filterAdapterFactory());
+
+  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
@@ -99,7 +94,7 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ISDK | SplitIO.IA
     // We will just log and allow for the SDK to end up throwing an SDK_TIMEOUT event for devs to handle.
     validateAndTrackApiKey(log, settings.core.authorizationKey);
     readiness.init();
-    uniqueKeysTracker && uniqueKeysTracker.start();
+    uniqueKeysTracker.start();
     syncManager && syncManager.start();
     signalListener && signalListener.start();
 

src/sdkFactory/types.ts

@@ -46,7 +46,7 @@ export interface ISdkFactoryContext {
   eventTracker: IEventTracker,
   telemetryTracker: ITelemetryTracker,
   storage: IStorageSync | IStorageAsync,
-  uniqueKeysTracker?: IUniqueKeysTracker,
+  uniqueKeysTracker: IUniqueKeysTracker,
   signalListener?: ISignalListener
   splitApi?: ISplitApi
   syncManager?: ISyncManager,

src/trackers/__tests__/impressionsTracker.spec.ts

@@ -36,6 +36,10 @@ const fakeSettingsWithListener = {
 };
 const fakeWhenInit = (cb: () => void) => cb();
 
+const fakeNoneStrategy = {
+  process: jest.fn(() => false)
+};
+
 /* Tests */
 
 describe('Impressions Tracker', () => {
@@ -48,15 +52,8 @@ describe('Impressions Tracker', () => {
 
   const strategy = strategyDebugFactory(impressionObserverCSFactory());
 
-  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);
-    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',
@@ -70,13 +67,13 @@ describe('Impressions Tracker', () => {
 
     expect(fakeImpressionsCache.track).not.toBeCalled(); // cache method should not be called by just creating a tracker
 
-    tracker.track([{ imp: imp1 }, { imp: imp2 }, { imp: imp3 }]);
+    tracker.track([{ imp: imp1 }, { imp: imp2, track: true }, { imp: imp3, track: false }]);
 
-    expect(fakeImpressionsCache.track.mock.calls[0][0]).toEqual([imp1, imp2, imp3]); // Should call the storage track method once we invoke .track() method, passing queued params in a sequence.
+    expect(fakeImpressionsCache.track.mock.calls[0][0]).toEqual([imp1, imp2]); // Should call the storage track method once we invoke .track() method, passing impressions with `track` enabled
   });
 
   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'
@@ -93,9 +90,9 @@ describe('Impressions Tracker', () => {
     expect(fakeIntegrationsManager.handleImpression).not.toBeCalled(); // The integrations manager handleImpression method should not be invoked if we haven't tracked impressions.
 
     // We signal that we actually want to track the queued impressions.
-    tracker.track([{ imp: fakeImpression }, { imp: fakeImpression2 }], fakeAttributes);
+    tracker.track([{ imp: fakeImpression }, { imp: fakeImpression2, track: false }], fakeAttributes);
 
-    expect(fakeImpressionsCache.track.mock.calls[0][0]).toEqual([fakeImpression, fakeImpression2]); // Even with a listener, impression should be sent to the cache
+    expect(fakeImpressionsCache.track.mock.calls[0][0]).toEqual([fakeImpression]); // Even with a listener, impressions (with `track` enabled) should be sent to the cache
     expect(fakeListener.logImpression).not.toBeCalled(); // The listener should not be executed synchronously.
     expect(fakeIntegrationsManager.handleImpression).not.toBeCalled(); // The integrations manager handleImpression method should not be executed synchronously.
 
@@ -150,14 +147,14 @@ 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.
 
     trackers.forEach(tracker => {
-      tracker.track([{ imp: impression }, { imp: impression2 }, { imp: impression3 }]);
+      tracker.track([{ imp: impression, track: true }, { imp: impression2 }, { imp: impression3 }]);
 
       const lastArgs = fakeImpressionsCache.track.mock.calls[fakeImpressionsCache.track.mock.calls.length - 1];
 
@@ -178,7 +175,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 +198,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([{ imp: 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,
@@ -28,41 +23,44 @@ export function impressionsTrackerFactory(
   const { log, impressionListener, runtime: { ip, hostname }, version } = settings;
 
   return {
-    track(imps: ImpressionDecorated[], attributes?: SplitIO.Attributes) {
+    track(impressions: ImpressionDecorated[], attributes?: SplitIO.Attributes) {
       if (settings.userConsent === CONSENT_DECLINED) return;
 
-      const impressions = imps.map((item) => item.imp);
-      const impressionsCount = impressions.length;
-      const { impressionsToStore, impressionsToListener, deduped } = strategy.process(impressions);
+      const impressionsToStore = impressions.filter(({ imp, track }) => {
+        return track === false ?
+          noneStrategy.process(imp) :
+          strategy.process(imp);
+      });
 
-      const impressionsToListenerCount = impressionsToListener.length;
+      const impressionsLength = impressions.length;
+      const impressionsToStoreLength = impressionsToStore.length;
 
-      if (impressionsToStore.length > 0) {
-        const res = impressionsCache.track(impressionsToStore);
+      if (impressionsToStoreLength) {
+        const res = impressionsCache.track(impressionsToStore.map((item) => item.imp));
 
         // 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].imp),
             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);
-
 });