feat: add ignoreMethods option (#920)

b-flightly · kjin · commit 67ddb8f126a1 · 2018-11-16T15:12:12.000-08:00
diff --git a/README.md b/README.md
@@ -36,6 +36,7 @@ Optionally, you can pass a [configuration object](src/config.ts) to the `start()
 require('@google-cloud/trace-agent').start({
   samplingRate: 5, // sample 5 traces per second, or at most 1 every 200 milliseconds.
   ignoreUrls: [ /^\/ignore-me#/ ] // ignore the "/ignore-me" endpoint.
+  ignoreMethods: [ 'options' ] // ignore requests with OPTIONS method (case-insensitive).
 });
 // ...
 ```
diff --git a/src/config.ts b/src/config.ts
@@ -134,6 +134,14 @@ export interface Config {
    */
   ignoreUrls?: Array<string|RegExp>;
 
+  /**
+   * Request methods that match any string in ignoreMethods will not be traced.
+   * matching is *not* case-sensitive (OPTIONS == options == OptiONs)
+   *
+   * No methods are ignored by default.
+   */
+  ignoreMethods?: string[];
+
   /**
    * An upper bound on the number of traces to gather each second. If set to 0,
    * sampling is disabled and all traces are recorded. Sampling rates greater
@@ -265,6 +273,7 @@ export const defaultConfig = {
   stackTraceLimit: 10,
   flushDelaySeconds: 30,
   ignoreUrls: ['/_ah/health'],
+  ignoreMethods: [],
   samplingRate: 10,
   contextHeaderBehavior: 'default',
   bufferSize: 1000,
diff --git a/src/plugin-types.ts b/src/plugin-types.ts
@@ -103,6 +103,8 @@ export interface SpanOptions {
 export interface RootSpanOptions extends SpanOptions {
   /* A URL associated with the root span, if applicable. */
   url?: string;
+  /* A Method associated with the root span, if applicable. */
+  method?: string;
   /**
    * The serialized form of an object that contains information about an
    * existing trace context, if it exists.
diff --git a/src/plugins/plugin-connect.ts b/src/plugins/plugin-connect.ts
@@ -42,6 +42,7 @@ function createMiddleware(api: PluginTypes.Tracer):
     const options = {
       name: req.originalUrl ? (urlParse(req.originalUrl).pathname || '') : '',
       url: req.originalUrl,
+      method: req.method,
       traceContext:
           getFirstHeader(req, api.constants.TRACE_CONTEXT_HEADER_NAME),
       skipFrames: 1
diff --git a/src/plugins/plugin-express.ts b/src/plugins/plugin-express.ts
@@ -39,6 +39,7 @@ function patchModuleRoot(express: Express4Module, api: PluginTypes.Tracer) {
       name: req.path,
       traceContext: req.get(api.constants.TRACE_CONTEXT_HEADER_NAME),
       url: req.originalUrl,
+      method: req.method,
       skipFrames: 1
     };
     api.runInRootSpan(options, (rootSpan) => {
diff --git a/src/plugins/plugin-hapi.ts b/src/plugins/plugin-hapi.ts
@@ -51,6 +51,7 @@ function instrument<T>(
   const options: PluginTypes.RootSpanOptions = {
     name: req.url ? (urlParse(req.url).pathname || '') : '',
     url: req.url,
+    method: req.method,
     traceContext: getFirstHeader(req, api.constants.TRACE_CONTEXT_HEADER_NAME),
     skipFrames: 2
   };
diff --git a/src/plugins/plugin-koa.ts b/src/plugins/plugin-koa.ts
@@ -56,6 +56,7 @@ function startSpanForRequest<T>(
   const options = {
     name: req.url ? (urlParse(req.url).pathname || '') : '',
     url: req.url,
+    method: req.method,
     traceContext: getFirstHeader(req, api.constants.TRACE_CONTEXT_HEADER_NAME),
     skipFrames: 2
   };
diff --git a/src/plugins/plugin-restify.ts b/src/plugins/plugin-restify.ts
@@ -51,6 +51,7 @@ function patchRestify(restify: Restify5, api: PluginTypes.Tracer) {
       // as a label later.
       name: req.path(),
       url: req.url,
+      method: req.method,
       traceContext: req.header(api.constants.TRACE_CONTEXT_HEADER_NAME),
       skipFrames: 1
     };
diff --git a/src/trace-api.ts b/src/trace-api.ts
@@ -192,8 +192,11 @@ export class StackdriverTracer implements Tracer {
     }
 
     // Consult the trace policy.
-    const locallyAllowed = this.policy!.shouldTrace(
-        {timestamp: Date.now(), url: options.url || ''});
+    const locallyAllowed = this.policy!.shouldTrace({
+      timestamp: Date.now(),
+      url: options.url || '',
+      method: options.method || ''
+    });
     const remotelyAllowed = !!(
         incomingTraceContext.options & Constants.TRACE_OPTIONS_TRACE_ENABLED);
 
diff --git a/src/tracing-policy.ts b/src/tracing-policy.ts
@@ -50,6 +50,16 @@ class URLFilter implements TracePolicyPredicate<string> {
   }
 }
 
+class MethodsFilter implements TracePolicyPredicate<string> {
+  constructor(private readonly filterMethods: string[]) {}
+
+  shouldTrace(method: string) {
+    return !this.filterMethods.some((candidate) => {
+      return (candidate.toLowerCase() === method.toLowerCase());
+    });
+  }
+}
+
 /**
  * Options for constructing a TracePolicy instance.
  */
@@ -62,6 +72,10 @@ export interface TracePolicyConfig {
    * A field that controls a url-based filter.
    */
   ignoreUrls: Array<string|RegExp>;
+  /**
+   * A field that controls a method filter.
+   */
+  ignoreMethods: string[];
 }
 
 /**
@@ -70,6 +84,7 @@ export interface TracePolicyConfig {
 export class TracePolicy {
   private readonly sampler: TracePolicyPredicate<number>;
   private readonly urlFilter: TracePolicyPredicate<string>;
+  private readonly methodsFilter: TracePolicyPredicate<string>;
 
   /**
    * Constructs a new TracePolicy instance.
@@ -88,23 +103,32 @@ export class TracePolicy {
     } else {
       this.urlFilter = new URLFilter(config.ignoreUrls);
     }
+    if (config.ignoreMethods.length === 0) {
+      this.methodsFilter = {shouldTrace: () => true};
+    } else {
+      this.methodsFilter = new MethodsFilter(config.ignoreMethods);
+    }
   }
 
   /**
    * Given a timestamp and URL, decides if a trace should be created.
    * @param options Fields that help determine whether a trace should be
    *                created.
    */
-  shouldTrace(options: {timestamp: number, url: string}): boolean {
+  shouldTrace(options: {timestamp: number, url: string, method: string}):
+      boolean {
     return this.sampler.shouldTrace(options.timestamp) &&
-        this.urlFilter.shouldTrace(options.url);
+        this.urlFilter.shouldTrace(options.url) &&
+        this.methodsFilter.shouldTrace(options.method);
   }
 
   static always(): TracePolicy {
-    return new TracePolicy({samplingRate: 0, ignoreUrls: []});
+    return new TracePolicy(
+        {samplingRate: 0, ignoreUrls: [], ignoreMethods: []});
   }
 
   static never(): TracePolicy {
-    return new TracePolicy({samplingRate: -1, ignoreUrls: []});
+    return new TracePolicy(
+        {samplingRate: -1, ignoreUrls: [], ignoreMethods: []});
   }
 }
diff --git a/test/plugins/common.ts b/test/plugins/common.ts
@@ -70,6 +70,7 @@ shimmer.wrap(trace, 'start', function(original) {
       rootSpanNameOverride: (name: string) => name,
       samplingRate: 0,
       ignoreUrls: [],
+      ignoreMethods: [],
       spansPerTraceSoftLimit: Infinity,
       spansPerTraceHardLimit: Infinity
     }, new TestLogger());
diff --git a/test/test-plugin-loader.ts b/test/test-plugin-loader.ts
@@ -46,6 +46,7 @@ describe('Trace Plugin Loader', () => {
             {
               samplingRate: 0,
               ignoreUrls: [],
+              ignoreMethods: [],
               enhancedDatabaseReporting: false,
               contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT,
               rootSpanNameOverride: (name: string) => name,
diff --git a/test/test-trace-api.ts b/test/test-trace-api.ts
@@ -39,6 +39,7 @@ describe('Trace Interface', () => {
               rootSpanNameOverride: (name: string) => name,
               samplingRate: 0,
               ignoreUrls: [],
+              ignoreMethods: [],
               spansPerTraceSoftLimit: Infinity,
               spansPerTraceHardLimit: Infinity
             },
@@ -222,6 +223,18 @@ describe('Trace Interface', () => {
           });
     });
 
+    it('should respect filter methods', () => {
+      const method = 'method';
+      const traceAPI = createTraceAgent({ignoreMethods: [method]});
+      traceAPI.runInRootSpan({name: 'root', method}, (rootSpan) => {
+        assert.strictEqual(rootSpan.type, SpanType.UNTRACED);
+      });
+      traceAPI.runInRootSpan(
+          {name: 'root', method: 'alternativeMethod'}, (rootSpan) => {
+            assert.strictEqual(rootSpan.type, SpanType.ROOT);
+          });
+    });
+
     it('should respect enhancedDatabaseReporting options field', () => {
       [true, false].forEach((enhancedDatabaseReporting) => {
         const traceAPI = createTraceAgent({
diff --git a/test/test-trace-policy.ts b/test/test-trace-policy.ts
@@ -21,16 +21,42 @@ import {TracePolicy} from '../src/tracing-policy';
 describe('TracePolicy', () => {
   describe('URL Filtering', () => {
     it('should not allow filtered urls', () => {
-      const policy = new TracePolicy(
-          {samplingRate: 0, ignoreUrls: ['/_ah/health', /\/book*/]});
-      assert.ok(!policy.shouldTrace({timestamp: 0, url: '/_ah/health'}));
-      assert.ok(!policy.shouldTrace({timestamp: 0, url: '/book/test'}));
+      const policy = new TracePolicy({
+        samplingRate: 0,
+        ignoreUrls: ['/_ah/health', /\/book*/],
+        ignoreMethods: []
+      });
+      assert.ok(
+          !policy.shouldTrace({timestamp: 0, url: '/_ah/health', method: ''}));
+      assert.ok(
+          !policy.shouldTrace({timestamp: 0, url: '/book/test', method: ''}));
     });
 
     it('should allow non-filtered urls', () => {
-      const policy =
-          new TracePolicy({samplingRate: 0, ignoreUrls: ['/_ah/health']});
-      assert.ok(policy.shouldTrace({timestamp: 0, url: '/_ah/background'}));
+      const policy = new TracePolicy(
+          {samplingRate: 0, ignoreUrls: ['/_ah/health'], ignoreMethods: []});
+      assert.ok(policy.shouldTrace(
+          {timestamp: 0, url: '/_ah/background', method: ''}));
+    });
+  });
+
+  describe('Method Filtering', () => {
+    it('should not allow filtered methods', () => {
+      const policy = new TracePolicy({
+        samplingRate: 0,
+        ignoreUrls: [],
+        ignoreMethods: ['method1', 'method2']
+      });
+      assert.ok(
+          !policy.shouldTrace({timestamp: 0, url: '', method: 'method1'}));
+      assert.ok(
+          !policy.shouldTrace({timestamp: 0, url: '', method: 'method2'}));
+    });
+
+    it('should allow non-filtered methods', () => {
+      const policy = new TracePolicy(
+          {samplingRate: 0, ignoreUrls: [], ignoreMethods: ['method']});
+      assert.ok(policy.shouldTrace({timestamp: 0, url: '', method: 'method1'}));
     });
   });
 
@@ -39,14 +65,14 @@ describe('TracePolicy', () => {
     const testCases = [0.1, 0.5, 1, 10, 50, 150, 200, 500, 1000];
     for (const testCase of testCases) {
       it(`should throttle traces when samplingRate = ` + testCase, () => {
-        const policy =
-            new TracePolicy({samplingRate: testCase, ignoreUrls: []});
+        const policy = new TracePolicy(
+            {samplingRate: testCase, ignoreUrls: [], ignoreMethods: []});
         const expected = NUM_SECONDS * testCase;
         let actual = 0;
         const start = Date.now();
         for (let timestamp = start; timestamp < start + 1000 * NUM_SECONDS;
              timestamp++) {
-          if (policy.shouldTrace({timestamp, url: ''})) {
+          if (policy.shouldTrace({timestamp, url: '', method: ''})) {
             actual++;
           }
         }
@@ -60,23 +86,25 @@ describe('TracePolicy', () => {
     }
 
     it('should always sample when samplingRate = 0', () => {
-      const policy = new TracePolicy({samplingRate: 0, ignoreUrls: []});
+      const policy =
+          new TracePolicy({samplingRate: 0, ignoreUrls: [], ignoreMethods: []});
       let numSamples = 0;
       const start = Date.now();
       for (let timestamp = start; timestamp < start + 1000; timestamp++) {
-        if (policy.shouldTrace({timestamp, url: ''})) {
+        if (policy.shouldTrace({timestamp, url: '', method: ''})) {
           numSamples++;
         }
       }
       assert.strictEqual(numSamples, 1000);
     });
 
     it('should never sample when samplingRate < 0', () => {
-      const policy = new TracePolicy({samplingRate: -1, ignoreUrls: []});
+      const policy = new TracePolicy(
+          {samplingRate: -1, ignoreUrls: [], ignoreMethods: []});
       let numSamples = 0;
       const start = Date.now();
       for (let timestamp = start; timestamp < start + 1000; timestamp++) {
-        if (policy.shouldTrace({timestamp, url: ''})) {
+        if (policy.shouldTrace({timestamp, url: '', method: ''})) {
           numSamples++;
         }
       }
diff --git a/test/test-trace-web-frameworks.ts b/test/test-trace-web-frameworks.ts
@@ -58,7 +58,7 @@ describe('Web framework tracing', () => {
   before(() => {
     testTraceModule.setCLSForTest();
     testTraceModule.setPluginLoaderForTest();
-    testTraceModule.start({ignoreUrls: [/ignore-me/]});
+    testTraceModule.start({ignoreUrls: [/ignore-me/], ignoreMethods: []});
     axios = require('axios');
   });
 
