Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add ignoreMethods option #920

Merged
merged 2 commits into from
Nov 16, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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-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,

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

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