Skip to content

Commit 5121dd8

Browse files
EmilianoSanchezEmilianoSanchez
authored
Merge pull request #479 from splitio/refactor-impressions-tracker
Refactor impressions tracker to simplify reusability
2 parents b102bbf + 0a226b7 commit 5121dd8

File tree

8 files changed

+87
-86
lines changed

8 files changed

+87
-86
lines changed

‎src/sdkClient/__tests__/sdkClientMethod.spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ const paramMocks = [
1818
settings: { mode: CONSUMER_MODE, log: loggerMock, core: { authorizationKey: 'sdk key '} },
1919
telemetryTracker: telemetryTrackerFactory(),
2020
clients: {},
21-
uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() },
21+
impressionsTracker: { start: jest.fn(), stop: jest.fn(), track: jest.fn() },
2222
fallbackTreatmentsCalculator: new FallbackTreatmentsCalculator({})
2323
},
2424
// SyncManager (i.e., Sync SDK) and Signal listener
@@ -30,7 +30,7 @@ const paramMocks = [
3030
settings: { mode: STANDALONE_MODE, log: loggerMock, core: { authorizationKey: 'sdk key '} },
3131
telemetryTracker: telemetryTrackerFactory(),
3232
clients: {},
33-
uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() },
33+
impressionsTracker: { start: jest.fn(), stop: jest.fn(), track: jest.fn() },
3434
fallbackTreatmentsCalculator: new FallbackTreatmentsCalculator({})
3535
}
3636
];
@@ -75,7 +75,7 @@ test.each(paramMocks)('sdkClientMethodFactory', (params, done: any) => {
7575
client.destroy().then(() => {
7676
expect(params.sdkReadinessManager.readinessManager.destroy).toBeCalledTimes(1);
7777
expect(params.storage.destroy).toBeCalledTimes(1);
78-
expect(params.uniqueKeysTracker.stop).toBeCalledTimes(1);
78+
expect(params.impressionsTracker.stop).toBeCalledTimes(1);
7979

8080
if (params.syncManager) {
8181
expect(params.syncManager.stop).toBeCalledTimes(1);

‎src/sdkClient/__tests__/sdkClientMethodCS.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ const params = {
4646
settings: settingsWithKey,
4747
telemetryTracker: telemetryTrackerFactory(),
4848
clients: {},
49-
uniqueKeysTracker: { start: jest.fn(), stop: jest.fn() }
49+
impressionsTracker: { start: jest.fn(), stop: jest.fn(), track: jest.fn() }
5050
};
5151

5252
const invalidAttributes = [
@@ -96,7 +96,7 @@ describe('sdkClientMethodCSFactory', () => {
9696
expect(params.syncManager.stop).toBeCalledTimes(1);
9797
expect(params.syncManager.flush).toBeCalledTimes(1);
9898
expect(params.signalListener.stop).toBeCalledTimes(1);
99-
expect(params.uniqueKeysTracker.stop).toBeCalledTimes(1);
99+
expect(params.impressionsTracker.stop).toBeCalledTimes(1);
100100
});
101101

102102
});

‎src/sdkClient/sdkClient.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const COOLDOWN_TIME_IN_MILLIS = 1000;
1111
* Creates an Sdk client, i.e., a base client with status, init, flush and destroy interface
1212
*/
1313
export function sdkClientFactory(params: ISdkFactoryContext, isSharedClient?: boolean): SplitIO.IClient | SplitIO.IAsyncClient {
14-
const { sdkReadinessManager, syncManager, storage, signalListener, settings, telemetryTracker, uniqueKeysTracker } = params;
14+
const { sdkReadinessManager, syncManager, storage, signalListener, settings, telemetryTracker, impressionsTracker } = params;
1515

1616
let hasInit = false;
1717
let lastActionTime = 0;
@@ -56,7 +56,7 @@ export function sdkClientFactory(params: ISdkFactoryContext, isSharedClient?: bo
5656
if (!isSharedClient) {
5757
validateAndTrackApiKey(settings.log, settings.core.authorizationKey);
5858
sdkReadinessManager.readinessManager.init();
59-
uniqueKeysTracker.start();
59+
impressionsTracker.start();
6060
syncManager && syncManager.start();
6161
signalListener && signalListener.start();
6262
}
@@ -77,7 +77,7 @@ export function sdkClientFactory(params: ISdkFactoryContext, isSharedClient?: bo
7777
releaseApiKey(settings.core.authorizationKey);
7878
telemetryTracker.sessionLength();
7979
signalListener && signalListener.stop();
80-
uniqueKeysTracker.stop();
80+
impressionsTracker.stop();
8181
}
8282

8383
// Stop background jobs

‎src/sdkFactory/index.ts‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,6 @@ import { createLoggerAPI } from '../logger/sdkLogger';
88
import { NEW_FACTORY, RETRIEVE_MANAGER } from '../logger/constants';
99
import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED, SDK_SPLITS_CACHE_LOADED } from '../readiness/constants';
1010
import { objectAssign } from '../utils/lang/objectAssign';
11-
import { strategyDebugFactory } from '../trackers/strategy/strategyDebug';
12-
import { strategyOptimizedFactory } from '../trackers/strategy/strategyOptimized';
13-
import { strategyNoneFactory } from '../trackers/strategy/strategyNone';
14-
import { uniqueKeysTrackerFactory } from '../trackers/uniqueKeysTracker';
15-
import { DEBUG, OPTIMIZED } from '../utils/constants';
1611
import { setRolloutPlan } from '../storages/setRolloutPlan';
1712
import { IStorageSync } from '../storages/types';
1813
import { getMatching } from '../utils/key';
@@ -24,10 +19,9 @@ import { FallbackTreatmentsCalculator } from '../evaluator/fallbackTreatmentsCal
2419
export function sdkFactory(params: ISdkFactoryParams): SplitIO.ISDK | SplitIO.IAsyncSDK | SplitIO.IBrowserSDK | SplitIO.IBrowserAsyncSDK {
2520

2621
const { settings, platform, storageFactory, splitApiFactory, extraProps,
27-
syncManagerFactory, SignalListener, impressionsObserverFactory,
28-
integrationsManagerFactory, sdkManagerFactory, sdkClientMethodFactory,
29-
filterAdapterFactory, lazyInit } = params;
30-
const { log, sync: { impressionsMode }, initialRolloutPlan, core: { key } } = settings;
22+
syncManagerFactory, SignalListener,
23+
integrationsManagerFactory, sdkManagerFactory, sdkClientMethodFactory, lazyInit } = params;
24+
const { log, initialRolloutPlan, core: { key } } = settings;
3125

3226
// @TODO handle non-recoverable errors, such as, global `fetch` not available, invalid SDK Key, etc.
3327
// On non-recoverable errors, we should mark the SDK as destroyed and not start synchronization.
@@ -62,23 +56,13 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ISDK | SplitIO.IA
6256
const telemetryTracker = telemetryTrackerFactory(storage.telemetry, platform.now);
6357
const integrationsManager = integrationsManagerFactory && integrationsManagerFactory({ settings, storage, telemetryTracker });
6458

65-
const observer = impressionsObserverFactory();
66-
const uniqueKeysTracker = uniqueKeysTrackerFactory(log, storage.uniqueKeys, filterAdapterFactory && filterAdapterFactory());
67-
68-
const noneStrategy = strategyNoneFactory(storage.impressionCounts, uniqueKeysTracker);
69-
const strategy = impressionsMode === OPTIMIZED ?
70-
strategyOptimizedFactory(observer, storage.impressionCounts) :
71-
impressionsMode === DEBUG ?
72-
strategyDebugFactory(observer) :
73-
noneStrategy;
74-
75-
const impressionsTracker = impressionsTrackerFactory(settings, storage.impressions, noneStrategy, strategy, integrationsManager, storage.telemetry);
59+
const impressionsTracker = impressionsTrackerFactory(params, storage, integrationsManager);
7660
const eventTracker = eventTrackerFactory(settings, storage.events, integrationsManager, storage.telemetry);
7761

7862
// splitApi is used by SyncManager and Browser signal listener
7963
const splitApi = splitApiFactory && splitApiFactory(settings, platform, telemetryTracker);
8064

81-
const ctx: ISdkFactoryContext = { clients, splitApi, eventTracker, impressionsTracker, telemetryTracker, uniqueKeysTracker, sdkReadinessManager, readiness, settings, storage, platform, fallbackTreatmentsCalculator };
65+
const ctx: ISdkFactoryContext = { clients, splitApi, eventTracker, impressionsTracker, telemetryTracker, sdkReadinessManager, readiness, settings, storage, platform, fallbackTreatmentsCalculator };
8266

8367
const syncManager = syncManagerFactory && syncManagerFactory(ctx as ISdkFactoryContextSync);
8468
ctx.syncManager = syncManager;

‎src/sdkFactory/types.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { IFetch, ISplitApi, IEventSourceConstructor } from '../services/types';
88
import { IStorageAsync, IStorageSync, IStorageFactoryParams } from '../storages/types';
99
import { ISyncManager } from '../sync/types';
1010
import { IImpressionObserver } from '../trackers/impressionObserver/types';
11-
import { IImpressionsTracker, IEventTracker, ITelemetryTracker, IFilterAdapter, IUniqueKeysTracker } from '../trackers/types';
11+
import { IImpressionsTracker, IEventTracker, ITelemetryTracker, IFilterAdapter } from '../trackers/types';
1212
import { ISettings } from '../types';
1313
import SplitIO from '../../types/splitio';
1414

@@ -47,7 +47,6 @@ export interface ISdkFactoryContext {
4747
eventTracker: IEventTracker,
4848
telemetryTracker: ITelemetryTracker,
4949
storage: IStorageSync | IStorageAsync,
50-
uniqueKeysTracker: IUniqueKeysTracker,
5150
signalListener?: ISignalListener
5251
splitApi?: ISplitApi
5352
syncManager?: ISyncManager,

‎src/trackers/__tests__/impressionsTracker.spec.ts‎

Lines changed: 37 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@ import { impressionsTrackerFactory } from '../impressionsTracker';
22
import { ImpressionCountsCacheInMemory } from '../../storages/inMemory/ImpressionCountsCacheInMemory';
33
import { impressionObserverSSFactory } from '../impressionObserver/impressionObserverSS';
44
import { impressionObserverCSFactory } from '../impressionObserver/impressionObserverCS';
5-
import SplitIO from '../../../types/splitio';
5+
import SplitIO, { ImpressionsMode } from '../../../types/splitio';
66
import { fullSettings } from '../../utils/settingsValidation/__tests__/settings.mocks';
7-
import { strategyDebugFactory } from '../strategy/strategyDebug';
8-
import { strategyOptimizedFactory } from '../strategy/strategyOptimized';
97
import { DEDUPED, QUEUED } from '../../utils/constants';
108

119
/* Mocks */
@@ -22,20 +20,22 @@ const fakeListener = {
2220
const fakeIntegrationsManager = {
2321
handleImpression: jest.fn()
2422
};
25-
const fakeSettings = {
26-
...fullSettings,
27-
runtime: {
28-
hostname: 'fake-hostname',
29-
ip: 'fake-ip'
30-
},
31-
version: 'jest-test'
23+
const fakeStorage = {
24+
impressions: fakeImpressionsCache,
25+
impressionCounts: new ImpressionCountsCacheInMemory(),
26+
uniqueKeys: { track: jest.fn() },
27+
telemetry: undefined
3228
};
33-
const fakeSettingsWithListener = {
34-
...fakeSettings,
35-
impressionListener: fakeListener
29+
const fakeParams = {
30+
settings: fullSettings,
31+
impressionsObserverFactory: impressionObserverCSFactory,
3632
};
37-
const fakeNoneStrategy = {
38-
process: jest.fn(() => false)
33+
const fakeParamsWithListener = {
34+
settings: {
35+
...fullSettings,
36+
impressionListener: fakeListener
37+
},
38+
impressionsObserverFactory: impressionObserverCSFactory,
3939
};
4040

4141
/* Tests */
@@ -48,10 +48,8 @@ describe('Impressions Tracker', () => {
4848
fakeIntegrationsManager.handleImpression.mockClear();
4949
});
5050

51-
const strategy = strategyDebugFactory(impressionObserverCSFactory());
52-
5351
test('Should be able to track impressions (in DEBUG mode without Previous Time).', () => {
54-
const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategy);
52+
const tracker = impressionsTrackerFactory(fakeParams, fakeStorage);
5553

5654
const imp1 = {
5755
feature: '10',
@@ -70,8 +68,8 @@ describe('Impressions Tracker', () => {
7068
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
7169
});
7270

73-
test('Tracked impressions should be sent to impression listener and integration manager when we invoke .track()', (done) => {
74-
const tracker = impressionsTrackerFactory(fakeSettingsWithListener, fakeImpressionsCache, fakeNoneStrategy, strategy, fakeIntegrationsManager);
71+
test('Tracked impressions should be sent to impression listener and integration manager when we invoke .track()', async () => {
72+
const tracker = impressionsTrackerFactory(fakeParamsWithListener, fakeStorage, fakeIntegrationsManager);
7573

7674
const fakeImpression = {
7775
feature: 'impression'
@@ -94,25 +92,23 @@ describe('Impressions Tracker', () => {
9492
expect(fakeListener.logImpression).not.toBeCalled(); // The listener should not be executed synchronously.
9593
expect(fakeIntegrationsManager.handleImpression).not.toBeCalled(); // The integrations manager handleImpression method should not be executed synchronously.
9694

97-
setTimeout(() => {
98-
expect(fakeListener.logImpression).toBeCalledTimes(2); // The listener should be executed after the timeout wrapping make it to the queue stack, once per each tracked impression.
99-
expect(fakeIntegrationsManager.handleImpression).toBeCalledTimes(2); // The integrations manager handleImpression method should be executed after the timeout wrapping make it to the queue stack, once per each tracked impression.
95+
await new Promise(resolve => setTimeout(resolve, 0));
10096

101-
const impressionData1 = { impression: fakeImpression, attributes: fakeAttributes, sdkLanguageVersion: fakeSettings.version, ip: fakeSettings.runtime.ip, hostname: fakeSettings.runtime.hostname };
102-
const impressionData2 = { impression: fakeImpression2, attributes: fakeAttributes, sdkLanguageVersion: fakeSettings.version, ip: fakeSettings.runtime.ip, hostname: fakeSettings.runtime.hostname };
97+
expect(fakeListener.logImpression).toBeCalledTimes(2); // The listener should be executed after the timeout wrapping make it to the queue stack, once per each tracked impression.
98+
expect(fakeIntegrationsManager.handleImpression).toBeCalledTimes(2); // The integrations manager handleImpression method should be executed after the timeout wrapping make it to the queue stack, once per each tracked impression.
10399

104-
expect(fakeListener.logImpression.mock.calls[0][0]).toEqual(impressionData1); // The listener should be executed with the corresponding map for each of the impressions.
105-
expect(fakeListener.logImpression.mock.calls[1][0]).toEqual(impressionData2); // The listener should be executed with the corresponding map for each of the impressions.
106-
expect(fakeListener.logImpression.mock.calls[0][0].impression).not.toBe(fakeImpression); // but impression should be a copy
107-
expect(fakeListener.logImpression.mock.calls[1][0].impression).not.toBe(fakeImpression2); // but impression should be a copy
100+
const impressionData1 = { impression: fakeImpression, attributes: fakeAttributes, sdkLanguageVersion: fullSettings.version, ip: fullSettings.runtime.ip, hostname: fullSettings.runtime.hostname };
101+
const impressionData2 = { impression: fakeImpression2, attributes: fakeAttributes, sdkLanguageVersion: fullSettings.version, ip: fullSettings.runtime.ip, hostname: fullSettings.runtime.hostname };
108102

109-
expect(fakeIntegrationsManager.handleImpression.mock.calls[0][0]).toEqual(impressionData1); // The integration manager handleImpression method should be executed with the corresponding map for each of the impressions.
110-
expect(fakeIntegrationsManager.handleImpression.mock.calls[1][0]).toEqual(impressionData2); // The integration manager handleImpression method should be executed with the corresponding map for each of the impressions.
111-
expect(fakeIntegrationsManager.handleImpression.mock.calls[0][0].impression).not.toBe(fakeImpression); // but impression should be a copy
112-
expect(fakeIntegrationsManager.handleImpression.mock.calls[1][0].impression).not.toBe(fakeImpression2); // but impression should be a copy
103+
expect(fakeListener.logImpression.mock.calls[0][0]).toEqual(impressionData1); // The listener should be executed with the corresponding map for each of the impressions.
104+
expect(fakeListener.logImpression.mock.calls[1][0]).toEqual(impressionData2); // The listener should be executed with the corresponding map for each of the impressions.
105+
expect(fakeListener.logImpression.mock.calls[0][0].impression).not.toBe(fakeImpression); // but impression should be a copy
106+
expect(fakeListener.logImpression.mock.calls[1][0].impression).not.toBe(fakeImpression2); // but impression should be a copy
113107

114-
done();
115-
}, 0);
108+
expect(fakeIntegrationsManager.handleImpression.mock.calls[0][0]).toEqual(impressionData1); // The integration manager handleImpression method should be executed with the corresponding map for each of the impressions.
109+
expect(fakeIntegrationsManager.handleImpression.mock.calls[1][0]).toEqual(impressionData2); // The integration manager handleImpression method should be executed with the corresponding map for each of the impressions.
110+
expect(fakeIntegrationsManager.handleImpression.mock.calls[0][0].impression).not.toBe(fakeImpression); // but impression should be a copy
111+
expect(fakeIntegrationsManager.handleImpression.mock.calls[1][0].impression).not.toBe(fakeImpression2); // but impression should be a copy
116112
});
117113

118114
const impression = {
@@ -145,8 +141,8 @@ describe('Impressions Tracker', () => {
145141
impression3.time = 1234567891;
146142

147143
const trackers = [
148-
impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyDebugFactory(impressionObserverSSFactory()), undefined),
149-
impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyDebugFactory(impressionObserverCSFactory()), undefined)
144+
impressionsTrackerFactory({ ...fakeParams, impressionsObserverFactory: impressionObserverSSFactory }, fakeStorage),
145+
impressionsTrackerFactory(fakeParams, fakeStorage)
150146
];
151147

152148
expect(fakeImpressionsCache.track).not.toBeCalled(); // storage method should not be called until impressions are tracked.
@@ -173,7 +169,7 @@ describe('Impressions Tracker', () => {
173169
impression3.time = Date.now();
174170

175171
const impressionCountsCache = new ImpressionCountsCacheInMemory();
176-
const tracker = impressionsTrackerFactory(fakeSettings, fakeImpressionsCache, fakeNoneStrategy, strategyOptimizedFactory(impressionObserverCSFactory(), impressionCountsCache), undefined, fakeTelemetryCache as any);
172+
const tracker = impressionsTrackerFactory(fakeParams, { ...fakeStorage, impressionCounts: impressionCountsCache, telemetry: fakeTelemetryCache } as any);
177173

178174
expect(fakeImpressionsCache.track).not.toBeCalled(); // cache method should not be called by just creating a tracker
179175

@@ -194,9 +190,10 @@ describe('Impressions Tracker', () => {
194190
});
195191

196192
test('Should track or not impressions depending on user consent status', () => {
197-
const settings = { ...fullSettings };
193+
const settings = { ...fullSettings, sync: { ...fullSettings.sync, impressionsMode: 'DEBUG' as ImpressionsMode } };
194+
const params = { settings, impressionsObserverFactory: impressionObserverCSFactory };
198195

199-
const tracker = impressionsTrackerFactory(settings, fakeImpressionsCache, fakeNoneStrategy, strategy);
196+
const tracker = impressionsTrackerFactory(params, fakeStorage);
200197

201198
tracker.track([{ imp: impression }]);
202199
expect(fakeImpressionsCache.track).toBeCalledTimes(1); // impression should be tracked if userConsent is undefined

0 commit comments

Comments
 (0)