fix: Produce a warning when track is called with a non-numeric metric value. (#449)

Co-authored-by: Yusinto Ngadiman <[email protected]>

Author: kinyoklion

packages/shared/common/src/internal/events/ClientMessages.ts

@@ -4,4 +4,11 @@
 export default class ClientMessages {
   static readonly missingContextKeyNoEvent =
     'Context was unspecified or had no key; event will not be sent';
+
+  static invalidMetricValue(badType: string) {
+    return (
+      'The track function was called with a non-numeric "metricValue"' +
+      ` (${badType}), only numeric metric values are supported.`
+    );
+  }
 }

packages/shared/sdk-client/src/LDClientImpl.events.test.ts

@@ -1,5 +1,5 @@
 import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common';
-import { InputIdentifyEvent } from '@launchdarkly/js-sdk-common/dist/internal';
+import { InputCustomEvent, InputIdentifyEvent } from '@launchdarkly/js-sdk-common/dist/internal';
 import {
   basicPlatform,
   hasher,
@@ -31,6 +31,7 @@ jest.mock('@launchdarkly/js-sdk-common', () => {
 const testSdkKey = 'test-sdk-key';
 let ldc: LDClientImpl;
 let defaultPutResponse: Flags;
+const carContext: LDContext = { kind: 'car', key: 'test-car' };
 
 describe('sdk-client object', () => {
   beforeEach(() => {
@@ -54,7 +55,6 @@ describe('sdk-client object', () => {
 
   test('identify event', async () => {
     defaultPutResponse['dev-test-flag'].value = false;
-    const carContext: LDContext = { kind: 'car', key: 'test-car' };
 
     await ldc.identify(carContext);
 
@@ -73,4 +73,85 @@ describe('sdk-client object', () => {
       }),
     );
   });
+
+  it('produces track events with data', async () => {
+    await ldc.identify(carContext);
+
+    ldc.track('the-event', { the: 'data' }, undefined);
+    expect(MockEventProcessor).toHaveBeenCalled();
+    expect(ldc.eventProcessor!.sendEvent).toHaveBeenNthCalledWith(
+      2,
+      expect.objectContaining<InputCustomEvent>({
+        kind: 'custom',
+        key: 'the-event',
+        context: expect.objectContaining({
+          contexts: expect.objectContaining({
+            car: { key: 'test-car' },
+          }),
+        }),
+        data: { the: 'data' },
+        samplingRatio: 1,
+        creationDate: expect.any(Number),
+      }),
+    );
+    expect(logger.warn).not.toHaveBeenCalled();
+  });
+
+  it('produces track events with a metric value', async () => {
+    await ldc.identify(carContext);
+
+    ldc.track('the-event', undefined, 12);
+    expect(MockEventProcessor).toHaveBeenCalled();
+    expect(ldc.eventProcessor!.sendEvent).toHaveBeenNthCalledWith(
+      2,
+      expect.objectContaining<InputCustomEvent>({
+        kind: 'custom',
+        key: 'the-event',
+        context: expect.objectContaining({
+          contexts: expect.objectContaining({
+            car: { key: 'test-car' },
+          }),
+        }),
+        metricValue: 12,
+        samplingRatio: 1,
+        creationDate: expect.any(Number),
+      }),
+    );
+    expect(logger.warn).not.toHaveBeenCalled();
+  });
+
+  it('produces track events with a metric value and data', async () => {
+    await ldc.identify(carContext);
+
+    ldc.track('the-event', { the: 'data' }, 12);
+    expect(MockEventProcessor).toHaveBeenCalled();
+    expect(ldc.eventProcessor!.sendEvent).toHaveBeenNthCalledWith(
+      2,
+      expect.objectContaining<InputCustomEvent>({
+        kind: 'custom',
+        key: 'the-event',
+        context: expect.objectContaining({
+          contexts: expect.objectContaining({
+            car: { key: 'test-car' },
+          }),
+        }),
+        metricValue: 12,
+        data: { the: 'data' },
+        samplingRatio: 1,
+        creationDate: expect.any(Number),
+      }),
+    );
+    expect(logger.warn).not.toHaveBeenCalled();
+  });
+
+  it('produces a warning when the metric value is non-numeric', async () => {
+    // @ts-ignore
+    await ldc.identify(carContext);
+    // @ts-ignore
+    ldc.track('the-event', { the: 'data' }, '12');
+
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringMatching(/was called with a non-numeric/),
+    );
+  });
 });

packages/shared/sdk-client/src/LDClientImpl.ts

@@ -402,6 +402,11 @@ export default class LDClientImpl implements LDClient {
       return;
     }
 
+    // 0 is valid, so do not truthy check the metric value
+    if (metricValue !== undefined && !TypeValidators.Number.is(metricValue)) {
+      this.logger?.warn(ClientMessages.invalidMetricValue(typeof metricValue));
+    }
+
     this.eventProcessor?.sendEvent(
       this.eventFactoryDefault.customEvent(key, checkedContext!, data, metricValue),
     );

packages/shared/sdk-server/__tests__/LDClient.events.test.ts

@@ -5,6 +5,7 @@ import * as mocks from '@launchdarkly/private-js-mocks';
 
 import { LDClientImpl } from '../src';
 import TestData from '../src/integrations/test_data/TestData';
+import TestLogger, { LogLevel } from './Logger';
 import makeCallbacks from './makeCallbacks';
 
 const defaultUser = { key: 'user' };
@@ -14,8 +15,10 @@ describe('given a client with mock event processor', () => {
   let client: LDClientImpl;
   let events: AsyncQueue<internal.InputEvent>;
   let td: TestData;
+  let logger: TestLogger;
 
   beforeEach(async () => {
+    logger = new TestLogger();
     events = new AsyncQueue<internal.InputEvent>();
     jest
       .spyOn(internal.EventProcessor.prototype, 'sendEvent')
@@ -30,6 +33,7 @@ describe('given a client with mock event processor', () => {
       mocks.basicPlatform,
       {
         updateProcessor: td.getFactory(),
+        logger,
       },
       makeCallbacks(false),
     );
@@ -300,4 +304,54 @@ describe('given a client with mock event processor', () => {
       default: 'c',
     });
   });
+
+  it('produces track events with data', async () => {
+    client.track('the-event', defaultUser, { the: 'data' }, undefined);
+    const e = await events.take();
+    expect(e).toMatchObject({
+      kind: 'custom',
+      key: 'the-event',
+      context: Context.fromLDContext(defaultUser),
+      data: { the: 'data' },
+    });
+    expect(logger.getCount(LogLevel.Warn)).toBe(0);
+  });
+
+  it('produces track events with a metric value', async () => {
+    client.track('the-event', defaultUser, undefined, 12);
+    const e = await events.take();
+    expect(e).toMatchObject({
+      kind: 'custom',
+      key: 'the-event',
+      context: Context.fromLDContext(defaultUser),
+      metricValue: 12,
+    });
+    expect(logger.getCount(LogLevel.Warn)).toBe(0);
+  });
+
+  it('produces track events with a metric value and data', async () => {
+    client.track('the-event', defaultUser, { the: 'data' }, 12);
+    const e = await events.take();
+    expect(e).toMatchObject({
+      kind: 'custom',
+      key: 'the-event',
+      context: Context.fromLDContext(defaultUser),
+      metricValue: 12,
+      data: { the: 'data' },
+    });
+    expect(logger.getCount(LogLevel.Warn)).toBe(0);
+  });
+
+  it('produces a warning when the metric value is non-numeric', async () => {
+    // @ts-ignore
+    client.track('the-event', defaultUser, { the: 'data' }, '12');
+    await events.take();
+    expect(logger.getCount(LogLevel.Warn)).toBe(1);
+    logger.expectMessages([
+      {
+        level: LogLevel.Warn,
+        matches: /was called with a non-numeric/,
+      },
+    ]);
+  });
 });

packages/shared/sdk-server/src/LDClientImpl.ts

@@ -714,6 +714,11 @@ export default class LDClientImpl implements LDClient {
       return;
     }
 
+    // 0 is valid, so do not truthy check the metric value
+    if (metricValue !== undefined && !TypeValidators.Number.is(metricValue)) {
+      this.logger?.warn(ClientMessages.invalidMetricValue(typeof metricValue));
+    }
+
     this.eventProcessor.sendEvent(
       this.eventFactoryDefault.customEvent(key, checkedContext!, data, metricValue),
     );