Skip to content

Commit

Permalink
feat: add ignoreMethods option (#920)
Browse files Browse the repository at this point in the history
  • Loading branch information
b-flightly authored and kjin committed Nov 16, 2018
1 parent 72b074d commit 67ddb8f
Show file tree
Hide file tree
Showing 15 changed files with 108 additions and 21 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
});
// ...
```
Expand Down
9 changes: 9 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -265,6 +273,7 @@ export const defaultConfig = {
stackTraceLimit: 10,
flushDelaySeconds: 30,
ignoreUrls: ['/_ah/health'],
ignoreMethods: [],
samplingRate: 10,
contextHeaderBehavior: 'default',
bufferSize: 1000,
Expand Down
2 changes: 2 additions & 0 deletions src/plugin-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/plugins/plugin-connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/plugins/plugin-express.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
1 change: 1 addition & 0 deletions src/plugins/plugin-hapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
1 change: 1 addition & 0 deletions src/plugins/plugin-koa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
1 change: 1 addition & 0 deletions src/plugins/plugin-restify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
7 changes: 5 additions & 2 deletions src/trace-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
32 changes: 28 additions & 4 deletions src/tracing-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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[];
}

/**
Expand All @@ -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.
Expand All @@ -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: []});
}
}
1 change: 1 addition & 0 deletions test/plugins/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ shimmer.wrap(trace, 'start', function(original) {
rootSpanNameOverride: (name: string) => name,
samplingRate: 0,
ignoreUrls: [],
ignoreMethods: [],
spansPerTraceSoftLimit: Infinity,
spansPerTraceHardLimit: Infinity
}, new TestLogger());
Expand Down
1 change: 1 addition & 0 deletions test/test-plugin-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ describe('Trace Plugin Loader', () => {
{
samplingRate: 0,
ignoreUrls: [],
ignoreMethods: [],
enhancedDatabaseReporting: false,
contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT,
rootSpanNameOverride: (name: string) => name,
Expand Down
13 changes: 13 additions & 0 deletions test/test-trace-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('Trace Interface', () => {
rootSpanNameOverride: (name: string) => name,
samplingRate: 0,
ignoreUrls: [],
ignoreMethods: [],
spansPerTraceSoftLimit: Infinity,
spansPerTraceHardLimit: Infinity
},
Expand Down Expand Up @@ -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({
Expand Down
56 changes: 42 additions & 14 deletions test/test-trace-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'}));
});
});

Expand All @@ -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++;
}
}
Expand All @@ -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++;
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/test-trace-web-frameworks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand Down

0 comments on commit 67ddb8f

Please sign in to comment.