From aefa689a3fc41b8de7e7f518789cadfdabb483b7 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Wed, 31 Oct 2018 14:53:36 -0700 Subject: [PATCH 1/3] feat: add contextHeaderBehavior option --- src/config.ts | 36 +++++++++---- src/index.ts | 12 ++++- src/trace-api.ts | 49 +++++++++++++---- src/tracing.ts | 2 +- test/plugins/common.ts | 4 +- test/test-config.ts | 105 ++++++++++++++++++++++++++----------- test/test-plugin-loader.ts | 3 +- test/test-trace-api.ts | 37 +++++++++---- 8 files changed, 181 insertions(+), 67 deletions(-) diff --git a/src/config.ts b/src/config.ts index 08761184b..a3b5f393c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -22,7 +22,12 @@ const pluginDirectory = export type CLSMechanism = 'async-hooks'|'async-listener'|'auto'|'none'|'singular'; -/** Available configuration options. */ +export type ContextHeaderBehavior = 'default'|'ignore'|'require'; + +/** + * Available configuration options. All fields are optional. See the + * defaultConfig object defined in this file for default assigned values. + */ export interface Config { /** * Log levels: 0=disabled, 1=error, 2=warn, 3=info, 4=debug @@ -137,6 +142,22 @@ export interface Config { */ samplingRate?: number; + /** + * Specifies how to use incoming trace context headers. The following options + * are available: + * 'default' -- Trace context headers will be respected for incoming + * requests that support it. A new trace will be created for requests + * without trace context headers. + * 'require' -- Same as default, but traces won't be created for requests + * without trace context headers. This should not be set for end user-facing + * services, as this header is usually set by other traced services rather + * than by users. + * 'ignore' -- Always ignore trace context headers, so a new trace with a + * unique ID will be created for every request. + * All traces are still subject to local tracing policy. + */ + contextHeaderBehavior?: ContextHeaderBehavior; + /** * The number of transactions we buffer before we publish to the trace * API, unless `flushDelaySeconds` seconds have elapsed first. @@ -166,13 +187,10 @@ export interface Config { onUncaughtException?: string; /** - * EXPERIMENTAL: - * Allows to ignore the requests X-Cloud-Trace-Context header if set. Setting - * this to true will cause traces generated by this module to appear - * separately from other distributed work done by other services on behalf of - * the same incoming request. Setting this will also cause sampling decisions - * made by other distributed components to be ignored. This is useful for - * aggregating traces generated by different cloud platform projects. + * Setting this to true or false is the same as setting contextHeaderBehavior + * to 'ignore' or 'default' respectively. If both are explicitly set, + * contextHeaderBehavior will be prioritized over this value. + * Deprecated: This option will be removed in a future release. */ ignoreContextHeader?: boolean; @@ -244,8 +262,8 @@ export const defaultConfig = { flushDelaySeconds: 30, ignoreUrls: ['/_ah/health'], samplingRate: 10, + contextHeaderBehavior: 'default', bufferSize: 1000, onUncaughtException: 'ignore', - ignoreContextHeader: false, serviceContext: {} }; diff --git a/src/index.ts b/src/index.ts index b4a0d8da2..614555db8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -60,6 +60,15 @@ function initConfig(projectConfig: Forceable): envSetConfig = require(path.resolve(process.env.GCLOUD_TRACE_CONFIG!)) as Config; } + + // Internally, ignoreContextHeader is no longer being used, so convert the + // user's value into a value for contextHeaderBehavior. But let this value + // be overridden by the user's explicitly set value for contextHeaderBehavior. + const contextHeaderBehaviorUnderride = { + contextHeaderBehavior: projectConfig.ignoreContextHeader ? 'ignore' : + 'default' + }; + // Configuration order of precedence: // 1. Environment Variables // 2. Project Config @@ -67,7 +76,8 @@ function initConfig(projectConfig: Forceable): // 4. Default Config (as specified in './config') const config = extend( true, {[FORCE_NEW]: projectConfig[FORCE_NEW]}, defaultConfig, - envSetConfig, projectConfig, envConfig, {plugins: {}}); + envSetConfig, contextHeaderBehaviorUnderride, projectConfig, envConfig, + {plugins: {}}); // The empty plugins object guarantees that plugins is a plain object, // even if it's explicitly specified in the config to be a non-object. diff --git a/src/trace-api.ts b/src/trace-api.ts index afaeab8a0..9f38938ab 100644 --- a/src/trace-api.ts +++ b/src/trace-api.ts @@ -28,13 +28,34 @@ import {traceWriter} from './trace-writer'; import {TracePolicy, TracePolicyConfig} from './tracing-policy'; import * as util from './util'; +/** + * An enumeration of the different possible types of behavior when dealing with + * incoming trace context. Requests are still subject to local tracing policy. + */ +export enum TraceContextHeaderBehavior { + /** + * Respect the trace context header if it exists; otherwise, trace the + * request as a new trace. + */ + DEFAULT = 'default', + /** + * Respect the trace context header if it exists; otherwise, treat the + * request as unsampled and don't trace it. + */ + REQUIRE = 'require', + /** + * Trace every request as a new trace, even if trace context exists. + */ + IGNORE = 'ignore' +} + /** * An interface describing configuration fields read by the StackdriverTracer * object. This includes fields read by the trace policy. */ export interface StackdriverTracerConfig extends TracePolicyConfig { enhancedDatabaseReporting: boolean; - ignoreContextHeader: boolean; + contextHeaderBehavior: TraceContextHeaderBehavior; rootSpanNameOverride: (path: string) => string; spansPerTraceSoftLimit: number; spansPerTraceHardLimit: number; @@ -43,7 +64,7 @@ export interface StackdriverTracerConfig extends TracePolicyConfig { interface IncomingTraceContext { traceId?: string; spanId?: string; - options?: number; + options: number; } /** @@ -151,20 +172,26 @@ export class StackdriverTracer implements Tracer { } // Attempt to read incoming trace context. - let incomingTraceContext: IncomingTraceContext = {}; - if (isString(options.traceContext) && !this.config!.ignoreContextHeader) { - const parsedContext = util.parseContextFromHeader(options.traceContext); - if (parsedContext) { - incomingTraceContext = parsedContext; - } + const incomingTraceContext: IncomingTraceContext = {options: 1}; + let parsedContext: util.TraceContext|null = null; + if (isString(options.traceContext) && + this.config!.contextHeaderBehavior !== + TraceContextHeaderBehavior.IGNORE) { + parsedContext = util.parseContextFromHeader(options.traceContext); + } + if (parsedContext) { + Object.assign(incomingTraceContext, parsedContext); + } else if ( + this.config!.contextHeaderBehavior === + TraceContextHeaderBehavior.REQUIRE) { + incomingTraceContext.options = 0; } // Consult the trace policy. const locallyAllowed = this.policy!.shouldTrace( {timestamp: Date.now(), url: options.url || ''}); - const remotelyAllowed = incomingTraceContext.options === undefined || - !!(incomingTraceContext.options & - Constants.TRACE_OPTIONS_TRACE_ENABLED); + const remotelyAllowed = !!( + incomingTraceContext.options & Constants.TRACE_OPTIONS_TRACE_ENABLED); let rootContext: RootSpan&RootContext; // Don't create a root span if the trace policy disallows it. diff --git a/src/tracing.ts b/src/tracing.ts index 43f3a038b..0f7401e17 100644 --- a/src/tracing.ts +++ b/src/tracing.ts @@ -32,7 +32,7 @@ export interface TopLevelConfig { // PluginLoaderConfig extends TraceAgentConfig export type NormalizedConfig = - (TraceWriterConfig&PluginLoaderConfig&TopLevelConfig)|{enabled: false}; + ((TraceWriterConfig&PluginLoaderConfig&TopLevelConfig)|{enabled: false}); /** * A class that represents automatic tracing. diff --git a/test/plugins/common.ts b/test/plugins/common.ts index b3befc2a3..4ca44db90 100644 --- a/test/plugins/common.ts +++ b/test/plugins/common.ts @@ -17,7 +17,7 @@ import '../override-gcp-metadata'; import { cls, TraceCLS } from '../../src/cls'; -import { StackdriverTracer } from '../../src/trace-api'; +import { StackdriverTracer, TraceContextHeaderBehavior } from '../../src/trace-api'; import { traceWriter } from '../../src/trace-writer'; import { SpanType } from '../../src/constants'; import { TestLogger } from '../logger'; @@ -66,7 +66,7 @@ shimmer.wrap(trace, 'start', function(original) { testTraceAgent = new StackdriverTracer('test'); testTraceAgent.enable({ enhancedDatabaseReporting: false, - ignoreContextHeader: false, + contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT, rootSpanNameOverride: (name: string) => name, samplingRate: 0, ignoreUrls: [], diff --git a/test/test-config.ts b/test/test-config.ts index 4ae3fdb24..8b8c34d1d 100644 --- a/test/test-config.ts +++ b/test/test-config.ts @@ -25,7 +25,7 @@ import { NormalizedConfig } from '../src/tracing'; import { StackdriverTracer } from '../src/trace-api'; import {Logger} from '../src/logger'; -describe('Behavior set by config for context propagation mechanism', () => { +describe('Behavior set by config for CLS', () => { const useAH = semver.satisfies(process.version, '>=8'); const autoMechanism = useAH ? TraceCLSMechanism.ASYNC_HOOKS : TraceCLSMechanism.ASYNC_LISTENER; @@ -93,9 +93,25 @@ describe('Behavior set by config for context propagation mechanism', () => { } }); -describe('Behavior set by config for overriding root span name', () => { +describe('Behavior set by config for Tracer', () => { let capturedConfig: NormalizedConfig|null; + // Convenience function to assert properties of capturedConfig that we want + // to be true on every test, and return an object with a conveniently + // sanitized type. + const getCapturedConfig = () => { + assert.ok(capturedConfig); + const config = capturedConfig!; + // If !config.enabled, then TSC does not permit access to other fields on + // config. So use this structure instead of assert.ok(config.enabled). + if (config.enabled) { + return config; + } else { + assert.fail('Configuration was not enabled.'); + throw new Error(); // unreachable. + } + }; + class CaptureConfigTestTracing extends testTraceModule.TestTracing { constructor(config: NormalizedConfig, traceAgent: StackdriverTracer) { super(config, traceAgent); @@ -116,40 +132,67 @@ describe('Behavior set by config for overriding root span name', () => { testTraceModule.setTracingForTest(testTraceModule.TestTracing); }); - it('should convert a string to a function', () => { - testTraceModule.start({ - rootSpanNameOverride: 'hello' + + describe('Context header behavior', () => { + it('should copy over an explicitly-set value', () => { + testTraceModule.start({ + contextHeaderBehavior: 'require' + }); + const config = getCapturedConfig(); + assert.strictEqual(config.contextHeaderBehavior, 'require'); + }); + + it('should respect the value of ignoreContextHeader if not set', () => { + testTraceModule.start({ + ignoreContextHeader: false + }); + let config = getCapturedConfig(); + assert.strictEqual(config.contextHeaderBehavior, 'default'); + capturedConfig = null; + testTraceModule.start({ + ignoreContextHeader: true + }); + config = getCapturedConfig(); + assert.strictEqual(config.contextHeaderBehavior, 'ignore'); + }); + + it('should override the value of ignoreContextHeader if both set', () => { + testTraceModule.start({ + ignoreContextHeader: false, + contextHeaderBehavior: 'require' + }); + let config = getCapturedConfig(); + assert.strictEqual(config.contextHeaderBehavior, 'require'); + capturedConfig = null; + testTraceModule.start({ + ignoreContextHeader: true, + contextHeaderBehavior: 'require' + }); + config = getCapturedConfig(); + assert.strictEqual(config.contextHeaderBehavior, 'require'); }); - assert.ok(capturedConfig!); - // Avoid using the ! operator multiple times. - const config = capturedConfig!; - // If !config.enabled, then TSC does not permit access to other fields on - // config. So use this structure instead of assert.ok(config.enabled). - if (config.enabled) { - assert.strictEqual(typeof config.rootSpanNameOverride, 'function'); - assert.strictEqual(config.rootSpanNameOverride(''), 'hello'); - } else { - assert.fail('Configuration was not enabled.'); - } }); - it('should convert a non-string, non-function to the identity fn', () => { - testTraceModule.start({ - // We should make sure passing in unsupported values at least doesn't - // result in a crash. - // tslint:disable-next-line:no-any - rootSpanNameOverride: 2 as any + describe('Overriding root span name', () => { + it('should convert a string to a function', () => { + testTraceModule.start({ + rootSpanNameOverride: 'hello' + }); + const config = getCapturedConfig(); + assert.strictEqual(typeof config.rootSpanNameOverride, 'function'); + assert.strictEqual(config.rootSpanNameOverride(''), 'hello'); }); - assert.ok(capturedConfig!); - // Avoid using the ! operator multiple times. - const config = capturedConfig!; - // If !config.enabled, then TSC does not permit access to other fields on - // config. So use this structure instead of assert.ok(config.enabled). - if (config.enabled) { + + it('should convert a non-string, non-function to the identity fn', () => { + testTraceModule.start({ + // We should make sure passing in unsupported values at least doesn't + // result in a crash. + // tslint:disable-next-line:no-any + rootSpanNameOverride: 2 as any + }); + const config = getCapturedConfig(); assert.strictEqual(typeof config.rootSpanNameOverride, 'function'); assert.strictEqual(config.rootSpanNameOverride('a'), 'a'); - } else { - assert.fail('Configuration was not enabled.'); - } + }); }); }); diff --git a/test/test-plugin-loader.ts b/test/test-plugin-loader.ts index 2dbc95662..a301e533c 100644 --- a/test/test-plugin-loader.ts +++ b/test/test-plugin-loader.ts @@ -20,6 +20,7 @@ import * as hook from 'require-in-the-middle'; import * as shimmer from 'shimmer'; import {Logger} from '../src/logger'; +import {TraceContextHeaderBehavior} from '../src/trace-api'; import {PluginLoader, PluginLoaderState, PluginWrapper} from '../src/trace-plugin-loader'; import {TestLogger} from './logger'; @@ -46,7 +47,7 @@ describe('Trace Plugin Loader', () => { samplingRate: 0, ignoreUrls: [], enhancedDatabaseReporting: false, - ignoreContextHeader: false, + contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT, rootSpanNameOverride: (name: string) => name, projectId: '0', spansPerTraceSoftLimit: Infinity, diff --git a/test/test-trace-api.ts b/test/test-trace-api.ts index 1abcc98cf..1d0c94006 100644 --- a/test/test-trace-api.ts +++ b/test/test-trace-api.ts @@ -19,7 +19,7 @@ import * as assert from 'assert'; import {cls, TraceCLS, TraceCLSMechanism} from '../src/cls'; import {defaultConfig} from '../src/config'; import {SpanType} from '../src/constants'; -import {StackdriverTracer, StackdriverTracerConfig} from '../src/trace-api'; +import {StackdriverTracer, StackdriverTracerConfig, TraceContextHeaderBehavior} from '../src/trace-api'; import {traceWriter} from '../src/trace-writer'; import {FORCE_NEW} from '../src/util'; @@ -35,7 +35,7 @@ describe('Trace Interface', () => { Object.assign( { enhancedDatabaseReporting: false, - ignoreContextHeader: false, + contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT, rootSpanNameOverride: (name: string) => name, samplingRate: 0, ignoreUrls: [], @@ -224,18 +224,22 @@ describe('Trace Interface', () => { it('should respect enhancedDatabaseReporting options field', () => { [true, false].forEach((enhancedDatabaseReporting) => { - const traceAPI = createTraceAgent( - {enhancedDatabaseReporting, ignoreContextHeader: false}); + const traceAPI = createTraceAgent({ + enhancedDatabaseReporting, + contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT + }); assert.strictEqual( traceAPI.enhancedDatabaseReportingEnabled(), enhancedDatabaseReporting); }); }); - it('should respect ignoreContextHeader options field', () => { - // ignoreContextHeader: true - createTraceAgent( - {enhancedDatabaseReporting: false, ignoreContextHeader: true}) + it('should respect contextHeaderBehavior options field', () => { + // ignore behavior + createTraceAgent({ + enhancedDatabaseReporting: false, + contextHeaderBehavior: TraceContextHeaderBehavior.IGNORE + }) .runInRootSpan( {name: 'root1', traceContext: '123456/667;o=1'}, (rootSpan) => { rootSpan.endSpan(); @@ -246,9 +250,11 @@ describe('Trace Interface', () => { assert.strictEqual(foundTrace.spans.length, 1); assert.strictEqual(foundTrace.spans[0].name, 'root1'); assert.notStrictEqual(foundTrace.spans[0].parentSpanId, '667'); - // ignoreContextHeader: false - createTraceAgent( - {enhancedDatabaseReporting: false, ignoreContextHeader: false}) + // default behavior + createTraceAgent({ + enhancedDatabaseReporting: false, + contextHeaderBehavior: TraceContextHeaderBehavior.DEFAULT + }) .runInRootSpan( {name: 'root2', traceContext: '123456/667;o=1'}, (rootSpan) => { rootSpan.endSpan(); @@ -258,6 +264,15 @@ describe('Trace Interface', () => { assert.strictEqual(foundTrace.spans.length, 1); assert.strictEqual(foundTrace.spans[0].name, 'root2'); assert.strictEqual(foundTrace.spans[0].parentSpanId, '667'); + // require behavior + createTraceAgent({ + enhancedDatabaseReporting: false, + contextHeaderBehavior: TraceContextHeaderBehavior.REQUIRE + }).runInRootSpan({name: 'root3'}, (rootSpan) => { + rootSpan.endSpan(); + }); + assert.strictEqual( + testTraceModule.getSpans(span => span.name === 'root3').length, 0); }); describe('getting response trace context', () => { From 937c88870d981dfd73f7d95ad35c09ccb6518ab3 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Thu, 1 Nov 2018 12:07:23 -0700 Subject: [PATCH 2/3] doc: update docs --- src/config.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/config.ts b/src/config.ts index a3b5f393c..46162432b 100644 --- a/src/config.ts +++ b/src/config.ts @@ -145,15 +145,18 @@ export interface Config { /** * Specifies how to use incoming trace context headers. The following options * are available: - * 'default' -- Trace context headers will be respected for incoming - * requests that support it. A new trace will be created for requests + * 'default' -- Trace context will be propagated for incoming requests that + * contain the context header. A new trace will be created for requests * without trace context headers. * 'require' -- Same as default, but traces won't be created for requests * without trace context headers. This should not be set for end user-facing * services, as this header is usually set by other traced services rather * than by users. - * 'ignore' -- Always ignore trace context headers, so a new trace with a - * unique ID will be created for every request. + * 'ignore' -- Trace context headers will always be ignored, so a new trace + * with a unique ID will be created for every request. This means that a + * sampling decision specified on an incoming request will be ignored. + * This might be useful for aggregating traces generated by different cloud + * platform projects. * All traces are still subject to local tracing policy. */ contextHeaderBehavior?: ContextHeaderBehavior; From e2c1d34086ae88d6764906474978bac48f906c2a Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Thu, 1 Nov 2018 14:48:49 -0700 Subject: [PATCH 3/3] doc: modify docs --- src/config.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/config.ts b/src/config.ts index 46162432b..526b1de67 100644 --- a/src/config.ts +++ b/src/config.ts @@ -147,7 +147,8 @@ export interface Config { * are available: * 'default' -- Trace context will be propagated for incoming requests that * contain the context header. A new trace will be created for requests - * without trace context headers. + * without trace context headers. All traces are still subject to local + * sampling and url filter policies. * 'require' -- Same as default, but traces won't be created for requests * without trace context headers. This should not be set for end user-facing * services, as this header is usually set by other traced services rather