fix: warn if tracePolicy conflicting options are specified

kjin · kjin · commit 389305961273 · 2019-05-14T11:34:41.000-07:00
diff --git a/src/index.ts b/src/index.ts
@@ -83,6 +83,7 @@ function initConfig(userConfig: Forceable<Config>): Forceable<TopLevelConfig> {
   return {
     [FORCE_NEW]: forceNew,
     enabled: mergedConfig.enabled,
+    original: userConfig,
     logLevel: lastOf(
         mergedConfig.logLevel, Number(process.env.GCLOUD_TRACE_LOGLEVEL)),
     clsConfig: {
diff --git a/src/tracing.ts b/src/tracing.ts
@@ -17,7 +17,7 @@
 import * as path from 'path';
 
 import {cls, TraceCLSConfig} from './cls';
-import {TracePolicy} from './config';
+import {Config, TracePolicy} from './config';
 import {LEVELS, Logger} from './logger';
 import {StackdriverTracer} from './trace-api';
 import {pluginLoader, PluginLoaderConfig} from './trace-plugin-loader';
@@ -26,7 +26,8 @@ import {BuiltinTracePolicy, TracePolicyConfig} from './tracing-policy';
 import {Component, Forceable, packageNameFromPath, Singleton} from './util';
 
 export type TopLevelConfig = {
-  enabled: boolean; logLevel: number; clsConfig: TraceCLSConfig;
+  original: Config; enabled: boolean; logLevel: number;
+  clsConfig: TraceCLSConfig;
   writerConfig: TraceWriterConfig;
   pluginLoaderConfig: PluginLoaderConfig;
   tracePolicyConfig: TracePolicyConfig;
@@ -120,6 +121,18 @@ export class Tracing implements Component {
 
     const tracePolicy = this.config.overrides.tracePolicy ||
         new BuiltinTracePolicy(this.config.tracePolicyConfig);
+    if (this.config.overrides.tracePolicy) {
+      const unusedOptions = Object.keys(this.config.tracePolicyConfig);
+      const optionsToWarn =
+          Object.keys(this.config.original)
+              .filter(key => unusedOptions.indexOf(key) !== -1)
+              .map(key => `config.${key}`);
+      if (optionsToWarn.length > 0) {
+        this.logger.warn(
+            'StackdriverTracer#start: config.tracePolicy was specified;',
+            `ignoring user-set values for: [${optionsToWarn.join(', ')}]`);
+      }
+    }
 
     this.traceAgent.enable(
         this.config.pluginLoaderConfig.tracerConfig, tracePolicy, this.logger);
diff --git a/test/test-config.ts b/test/test-config.ts
@@ -154,3 +154,14 @@ describe('Behavior set by config for Tracer', () => {
     });
   });
 });
+
+describe('Warnings for improper config', () => {
+  it('should indicate that some options can\'t be specified with a tracePolicy', () => {
+    testTraceModule.start({
+      samplingRate: 100,
+      tracePolicy: { shouldTrace: () => true }
+    });
+    const expectedWarnMsg = /config.tracePolicy.*ignoring.*\[config.samplingRate\]/;
+    assert.strictEqual(testTraceModule.getLogger().getNumLogsWith('warn', expectedWarnMsg), 1);
+  });
+});
diff --git a/test/trace.ts b/test/trace.ts
@@ -47,19 +47,27 @@ import {TraceWriter, traceWriter, TraceWriterConfig} from '../src/trace-writer';
 import {tracing, Tracing} from '../src/tracing';
 import {FORCE_NEW} from '../src/util';
 
-import {TestLogger} from './logger';
+import * as testLogger from './logger';
 
 export {Config, PluginTypes};
 
 const traces: Trace[] = [];
 const spans: TraceSpan[] = [];
+let capturedLogger: testLogger.TestLogger|null = null;
 
 export class TestCLS extends TraceCLS {
   constructor(config: {}, logger: logger.Logger) {
     super({mechanism: TraceCLSMechanism.NONE}, logger);
   }
 }
 
+export class TestLogger extends testLogger.TestLogger {
+  constructor(options?: Partial<logger.LoggerConfig>) {
+    super(options);
+    capturedLogger = this;
+  }
+}
+
 export class TestTraceWriter extends TraceWriter {
   constructor(config: TraceWriterConfig, logger: logger.Logger) {
     super(config, logger);
@@ -98,6 +106,7 @@ setTracingForTest(TestTracing);
 export type Predicate<T> = (value: T) => boolean;
 
 export function start(projectConfig?: Config): PluginTypes.Tracer {
+  capturedLogger = null;
   const agent = trace.start(Object.assign(
       {samplingRate: 0, logLevel: 4, [FORCE_NEW]: true}, projectConfig));
   return agent;
@@ -133,6 +142,11 @@ export function setTracingForTest(impl?: typeof Tracing) {
   tracing['implementation'] = impl || Tracing;
 }
 
+export function getLogger(): testLogger.TestLogger {
+  assert.ok(capturedLogger);
+  return capturedLogger!;
+}
+
 export function getTraces(predicate?: Predicate<Trace>): Trace[] {
   if (!predicate) {
     predicate = () => true;
