[sc-170667] Add basic logging support. (#4)

Author: kinyoklion

__tests__/LaunchDarklyProvider.test.ts

@@ -2,20 +2,23 @@ import { OpenFeature, Client } from '@openfeature/js-sdk';
 import { LDClient } from 'launchdarkly-node-server-sdk';
 import { LaunchDarklyProvider } from '../src';
 import translateContext from '../src/translateContext';
+import TestLogger from './TestLogger';
 
 const basicContext = { targetingKey: 'the-key' };
 const testFlagKey = 'a-key';
 
 describe('given a mock LaunchDarkly client', () => {
   let ldClient: LDClient;
   let ofClient: Client;
+  const logger: TestLogger = new TestLogger();
 
   beforeEach(() => {
     ldClient = {
       variationDetail: jest.fn(),
     } as any;
-    OpenFeature.setProvider(new LaunchDarklyProvider(ldClient));
+    OpenFeature.setProvider(new LaunchDarklyProvider(ldClient, { logger }));
     ofClient = OpenFeature.getClient();
+    logger.reset();
   });
 
   it('calls the client correctly for boolean variations', async () => {
@@ -27,11 +30,11 @@ describe('given a mock LaunchDarkly client', () => {
     }));
     await ofClient.getBooleanDetails(testFlagKey, false, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), false);
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), false);
     jest.clearAllMocks();
     await ofClient.getBooleanValue(testFlagKey, false, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), false);
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), false);
   });
 
   it('handles correct return types for boolean variations', async () => {
@@ -74,11 +77,11 @@ describe('given a mock LaunchDarkly client', () => {
     }));
     await ofClient.getStringDetails(testFlagKey, 'default', basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), 'default');
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), 'default');
     jest.clearAllMocks();
     await ofClient.getStringValue(testFlagKey, 'default', basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), 'default');
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), 'default');
   });
 
   it('handles correct return types for string variations', async () => {
@@ -121,11 +124,11 @@ describe('given a mock LaunchDarkly client', () => {
     }));
     await ofClient.getNumberDetails(testFlagKey, 0, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), 0);
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), 0);
     jest.clearAllMocks();
     await ofClient.getNumberValue(testFlagKey, 0, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), 0);
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), 0);
   });
 
   it('handles correct return types for numeric variations', async () => {
@@ -168,11 +171,11 @@ describe('given a mock LaunchDarkly client', () => {
     }));
     await ofClient.getObjectDetails(testFlagKey, {}, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), {});
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), {});
     jest.clearAllMocks();
     await ofClient.getObjectValue(testFlagKey, {}, basicContext);
     expect(ldClient.variationDetail)
-      .toHaveBeenCalledWith(testFlagKey, translateContext(basicContext), {});
+      .toHaveBeenCalledWith(testFlagKey, translateContext(logger, basicContext), {});
   });
 
   it('handles correct return types for object variations', async () => {
@@ -246,4 +249,16 @@ describe('given a mock LaunchDarkly client', () => {
       reason: 'OFF',
     });
   });
+
+  it('logs information about missing keys', async () => {
+    await ofClient.getObjectDetails(testFlagKey, {}, {});
+    expect(logger.logs[0]).toEqual("The EvaluationContext must contain either a 'targetingKey' "
+    + "or a 'key' and the type must be a string.");
+  });
+
+  it('logs information about double keys', async () => {
+    await ofClient.getObjectDetails(testFlagKey, {}, { targetingKey: '1', key: '2' });
+    expect(logger.logs[0]).toEqual("The EvaluationContext contained both a 'targetingKey' and a"
+    + " 'key' attribute. The 'key' attribute will be discarded.");
+  });
 });

__tests__/SafeLogger.test.ts

@@ -0,0 +1,36 @@
+import { basicLogger, LDLogger } from 'launchdarkly-node-server-sdk';
+import SafeLogger from '../src/SafeLogger';
+
+it('throws when constructed with an invalid logger', () => {
+  expect(
+    () => new SafeLogger({} as LDLogger, basicLogger({})),
+  ).toThrow();
+});
+
+describe('given a logger that throws in logs', () => {
+  const strings: string[] = [];
+  const logger = new SafeLogger({
+    info: () => { throw new Error('info'); },
+    debug: () => { throw new Error('info'); },
+    warn: () => { throw new Error('info'); },
+    error: () => { throw new Error('info'); },
+  }, basicLogger({
+    level: 'debug',
+    destination: (...args: any) => {
+      strings.push(args.join(' '));
+    },
+  }));
+
+  it('uses the fallback logger', () => {
+    logger.debug('a');
+    logger.info('b');
+    logger.warn('c');
+    logger.error('d');
+    expect(strings).toEqual([
+      'debug: [LaunchDarkly] a',
+      'info: [LaunchDarkly] b',
+      'warn: [LaunchDarkly] c',
+      'error: [LaunchDarkly] d',
+    ]);
+  });
+});

__tests__/TestLogger.ts

@@ -0,0 +1,25 @@
+import { LDLogger } from 'launchdarkly-node-server-sdk';
+
+export default class TestLogger implements LDLogger {
+  public logs: string[] = [];
+
+  error(...args: any[]): void {
+    this.logs.push(args.join(' '));
+  }
+
+  warn(...args: any[]): void {
+    this.logs.push(args.join(' '));
+  }
+
+  info(...args: any[]): void {
+    this.logs.push(args.join(' '));
+  }
+
+  debug(...args: any[]): void {
+    this.logs.push(args.join(' '));
+  }
+
+  reset() {
+    this.logs = [];
+  }
+}

__tests__/translateContext.test.ts

@@ -1,7 +1,10 @@
 import translateContext from '../src/translateContext';
+import TestLogger from './TestLogger';
 
 it('Uses the targetingKey as the user key', () => {
-  expect(translateContext({ targetingKey: 'the-key' })).toEqual({ key: 'the-key' });
+  const logger = new TestLogger();
+  expect(translateContext(logger, { targetingKey: 'the-key' })).toEqual({ key: 'the-key' });
+  expect(logger.logs.length).toEqual(0);
 });
 
 describe.each([
@@ -15,13 +18,16 @@ describe.each([
   ['country', 'value8'],
   ['anonymous', true],
 ])('given correct built-in attributes', (key, value) => {
+  const logger = new TestLogger();
   it('translates the key correctly', () => {
     expect(translateContext(
+      logger,
       { targetingKey: 'the-key', [key]: value },
     )).toEqual({
       key: 'the-key',
       [key]: value,
     });
+    expect(logger.logs.length).toEqual(0);
   });
 });
 
@@ -37,31 +43,39 @@ describe.each([
   ['anonymous', 'value'],
 ])('given incorrect built-in attributes', (key, value) => {
   it('the bad key is omitted', () => {
+    const logger = new TestLogger();
     expect(translateContext(
+      logger,
       { targetingKey: 'the-key', [key]: value },
     )).toEqual({
       key: 'the-key',
     });
+    expect(logger.logs[0]).toMatch(new RegExp(`The attribute '${key}' must be of type.*`));
   });
 });
 
 it('accepts custom attributes', () => {
-  expect(translateContext({ targetingKey: 'the-key', someAttr: 'someValue' })).toEqual({
+  const logger = new TestLogger();
+  expect(translateContext(logger, { targetingKey: 'the-key', someAttr: 'someValue' })).toEqual({
     key: 'the-key',
     custom: {
       someAttr: 'someValue',
     },
   });
+  expect(logger.logs.length).toEqual(0);
 });
 
 it('ignores custom attributes that are objects', () => {
-  expect(translateContext({ targetingKey: 'the-key', someAttr: {} })).toEqual({
+  const logger = new TestLogger();
+  expect(translateContext(logger, { targetingKey: 'the-key', someAttr: {} })).toEqual({
     key: 'the-key',
   });
+  expect(logger.logs[0]).toEqual("The attribute 'someAttr' is of an unsupported type 'object'");
 });
 
 it('accepts string/boolean/number arrays', () => {
-  expect(translateContext({
+  const logger = new TestLogger();
+  expect(translateContext(logger, {
     targetingKey: 'the-key',
     strings: ['a', 'b', 'c'],
     numbers: [1, 2, 3],
@@ -74,24 +88,34 @@ it('accepts string/boolean/number arrays', () => {
       booleans: [true, false],
     },
   });
+  expect(logger.logs.length).toEqual(0);
 });
 
 it('discards invalid array types', () => {
-  expect(translateContext({
-    targetingKey: 'the-key',
-    mixedTypes: [true, 'b', 1],
-    dates: [new Date()],
-  })).toEqual({
+  const logger = new TestLogger();
+  expect(translateContext(
+    logger,
+    {
+      targetingKey: 'the-key',
+      dates: [new Date()],
+    },
+  )).toEqual({
     key: 'the-key',
   });
+  expect(logger.logs[0]).toEqual("The attribute 'dates' is an unsupported array type.");
 });
 
 it('converts date to ISO strings', () => {
   const date = new Date();
-  expect(translateContext({ targetingKey: 'the-key', date })).toEqual({
+  const logger = new TestLogger();
+  expect(translateContext(
+    logger,
+    { targetingKey: 'the-key', date },
+  )).toEqual({
     key: 'the-key',
     custom: {
       date: date.toISOString(),
     },
   });
+  expect(logger.logs.length).toEqual(0);
 });

src/LaunchDarklyProvider.ts

@@ -3,7 +3,10 @@ import {
   JsonValue,
   Provider, ProviderMetadata, ResolutionDetails,
 } from '@openfeature/js-sdk';
-import { LDClient } from 'launchdarkly-node-server-sdk';
+import {
+  basicLogger, LDClient, LDLogger,
+} from 'launchdarkly-node-server-sdk';
+import { LaunchDarklyProviderOptions } from './LaunchDarklyProviderOptions';
 import translateContext from './translateContext';
 import translateResult from './translateResult';
 
@@ -25,6 +28,8 @@ function wrongTypeResult<T>(value: T): ResolutionDetails<T> {
  * An OpenFeature provider for the LaunchDarkly SDK for node.
  */
 export default class LaunchDarklyProvider implements Provider {
+  private readonly logger: LDLogger;
+
   readonly metadata: ProviderMetadata = {
     name: 'launchdarkly-node-provider',
   };
@@ -33,7 +38,12 @@ export default class LaunchDarklyProvider implements Provider {
    * Construct a {@link LaunchDarklyProvider}.
    * @param client The LaunchDarkly client instance to use.
    */
-  constructor(private readonly client: LDClient) {
+  constructor(private readonly client: LDClient, options: LaunchDarklyProviderOptions = {}) {
+    if (options.logger) {
+      this.logger = options.logger;
+    } else {
+      this.logger = basicLogger({ level: 'info' });
+    }
   }
 
   /**
@@ -54,7 +64,11 @@ export default class LaunchDarklyProvider implements Provider {
     defaultValue: boolean,
     context: EvaluationContext,
   ): Promise<ResolutionDetails<boolean>> {
-    const res = await this.client.variationDetail(flagKey, translateContext(context), defaultValue);
+    const res = await this.client.variationDetail(
+      flagKey,
+      this.translateContext(context),
+      defaultValue,
+    );
     if (typeof res.value === 'boolean') {
       return translateResult(res);
     }
@@ -79,7 +93,11 @@ export default class LaunchDarklyProvider implements Provider {
     defaultValue: string,
     context: EvaluationContext,
   ): Promise<ResolutionDetails<string>> {
-    const res = await this.client.variationDetail(flagKey, translateContext(context), defaultValue);
+    const res = await this.client.variationDetail(
+      flagKey,
+      this.translateContext(context),
+      defaultValue,
+    );
     if (typeof res.value === 'string') {
       return translateResult(res);
     }
@@ -104,7 +122,11 @@ export default class LaunchDarklyProvider implements Provider {
     defaultValue: number,
     context: EvaluationContext,
   ): Promise<ResolutionDetails<number>> {
-    const res = await this.client.variationDetail(flagKey, translateContext(context), defaultValue);
+    const res = await this.client.variationDetail(
+      flagKey,
+      this.translateContext(context),
+      defaultValue,
+    );
     if (typeof res.value === 'number') {
       return translateResult(res);
     }
@@ -127,7 +149,11 @@ export default class LaunchDarklyProvider implements Provider {
     defaultValue: U,
     context: EvaluationContext,
   ): Promise<ResolutionDetails<U>> {
-    const res = await this.client.variationDetail(flagKey, translateContext(context), defaultValue);
+    const res = await this.client.variationDetail(
+      flagKey,
+      this.translateContext(context),
+      defaultValue,
+    );
     if (typeof res.value === 'object') {
       return translateResult(res);
     }
@@ -138,4 +164,8 @@ export default class LaunchDarklyProvider implements Provider {
   get hooks(): Hook<FlagValue>[] {
     return [];
   }
+
+  private translateContext(context: EvaluationContext) {
+    return translateContext(this.logger, context);
+  }
 }

src/LaunchDarklyProviderOptions.ts

@@ -0,0 +1,23 @@
+import { LDLogger } from 'launchdarkly-node-server-sdk';
+
+/**
+ * Options for the {@link LaunchDarklyProvider}.
+ */
+export interface LaunchDarklyProviderOptions {
+  /**
+   * Configures a logger for warnings and errors generated by the provider.
+   *
+   * The logger can be any object that conforms to the `LDLogger` interface.
+   * For a simple implementation that lets you filter by log level, see
+   * `basicLogger`. You can also use an instance of `winston.Logger` from
+   * the Winston logging package.
+   *
+   * If you do not set this property, the provider uses `basicLogger` with a
+   * minimum level of `info`.
+   *
+   * By default this will not be the same logger instance used by the SDK.
+   * If the provider should share a logger with the SDK, then you will need to
+   * provide the logger in the SDK options as well.
+   */
+  logger?: LDLogger;
+}