From 7741963bc407d912aa19d6bf3f335bfdc2e518db Mon Sep 17 00:00:00 2001 From: Michael Goin Date: Fri, 7 Aug 2020 11:23:17 -0700 Subject: [PATCH 1/3] fix: fixed handling of extra traceparent fields for version 00 --- .../context/propagation/HttpTraceContext.ts | 36 +++++++++++++------ .../test/context/HttpTraceContext.test.ts | 13 +++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts b/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts index 2c732e6cd7..3a46f26226 100644 --- a/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts +++ b/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts @@ -27,9 +27,14 @@ import { getParentSpanContext, setExtractedSpanContext } from '../context'; export const TRACE_PARENT_HEADER = 'traceparent'; export const TRACE_STATE_HEADER = 'tracestate'; -const VALID_TRACE_PARENT_REGEX = /^(?!ff)[\da-f]{2}-([\da-f]{32})-([\da-f]{16})-([\da-f]{2})(-|$)/; + const VERSION = '00'; +const VERSION_REGEX = /^(?!ff)[\da-f]{2}$/; +const TRACE_ID_REGEX = /^(?![0]{32})[\da-f]{32}$/; +const PARENT_ID_REGEX = /^(?![0]{16})[\da-f]{16}$/; +const FLAGS_REGEX = /^[\da-f]{2}$/; + /** * Parses information from the [traceparent] span tag and converts it into {@link SpanContext} * @param traceParent - A meta property that comes from server. @@ -41,19 +46,30 @@ const VERSION = '00'; * For more information see {@link https://www.w3.org/TR/trace-context/} */ export function parseTraceParent(traceParent: string): SpanContext | null { - const match = traceParent.match(VALID_TRACE_PARENT_REGEX); - if ( - !match || - match[1] === '00000000000000000000000000000000' || - match[2] === '0000000000000000' - ) { + const trimmed = traceParent.trim(); + const traceParentParts = trimmed.split('-'); + + // Version 00 only allows the specific 4 fields. + // For future versions, we can grab just the parts (4 fields) we support. + if (traceParentParts[0] === VERSION && traceParentParts.length !== 4) { + return null; + } + + const [version, traceId, parentId, flags] = traceParentParts; + const isValidParent = + VERSION_REGEX.test(version) && + TRACE_ID_REGEX.test(traceId) && + PARENT_ID_REGEX.test(parentId) && + FLAGS_REGEX.test(flags); + + if (!isValidParent) { return null; } return { - traceId: match[1], - spanId: match[2], - traceFlags: parseInt(match[3], 16), + traceId: traceId, + spanId: parentId, + traceFlags: parseInt(flags, 16), }; } diff --git a/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts b/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts index a4602e7da8..f7c472a982 100644 --- a/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts +++ b/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts @@ -147,6 +147,19 @@ describe('HttpTraceContext', () => { ); }); + it('should return null if matching version but extra fields (invalid)', () => { + // Version 00 (our current) consists of {version}-{traceId}-{parentId}-{flags} + carrier[TRACE_PARENT_HEADER] = + '00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01-extra'; + + assert.deepStrictEqual( + getExtractedSpanContext( + httpTraceContext.extract(Context.ROOT_CONTEXT, carrier, defaultGetter) + ), + undefined + ); + }); + it('extracts traceparent from list of header', () => { carrier[TRACE_PARENT_HEADER] = [ '00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01', From 18b4c5dbced757ad1866894c5a3dd12cfa747dee Mon Sep 17 00:00:00 2001 From: Michael Goin Date: Fri, 7 Aug 2020 12:03:39 -0700 Subject: [PATCH 2/3] fix: fixes W3C tracestate OWS handling --- packages/opentelemetry-core/src/trace/TraceState.ts | 7 ++++--- .../test/context/HttpTraceContext.test.ts | 13 +++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-core/src/trace/TraceState.ts b/packages/opentelemetry-core/src/trace/TraceState.ts index 0ef420dbe3..c7c18802e7 100644 --- a/packages/opentelemetry-core/src/trace/TraceState.ts +++ b/packages/opentelemetry-core/src/trace/TraceState.ts @@ -68,10 +68,11 @@ export class TraceState implements api.TraceState { .split(LIST_MEMBERS_SEPARATOR) .reverse() // Store in reverse so new keys (.set(...)) will be placed at the beginning .reduce((agg: Map, part: string) => { - const i = part.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER); + const listMember = part.trim(); // Optional Whitespace (OWS) handling + const i = listMember.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER); if (i !== -1) { - const key = part.slice(0, i); - const value = part.slice(i + 1, part.length); + const key = listMember.slice(0, i); + const value = listMember.slice(i + 1, part.length); if (validateKey(key) && validateValue(value)) { agg.set(key, value); } else { diff --git a/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts b/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts index f7c472a982..cf2852112f 100644 --- a/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts +++ b/packages/opentelemetry-core/test/context/HttpTraceContext.test.ts @@ -258,6 +258,19 @@ describe('HttpTraceContext', () => { }); }); + it('should handle OWS in tracestate list members', () => { + carrier[TRACE_PARENT_HEADER] = + '00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01'; + carrier[TRACE_STATE_HEADER] = 'foo=1 \t , \t bar=2, \t baz=3 '; + const extractedSpanContext = getExtractedSpanContext( + httpTraceContext.extract(Context.ROOT_CONTEXT, carrier, defaultGetter) + ); + + assert.deepStrictEqual(extractedSpanContext!.traceState!.get('foo'), '1'); + assert.deepStrictEqual(extractedSpanContext!.traceState!.get('bar'), '2'); + assert.deepStrictEqual(extractedSpanContext!.traceState!.get('baz'), '3'); + }); + it('should fail gracefully on bad responses from getter', () => { const ctx1 = httpTraceContext.extract( Context.ROOT_CONTEXT, From 0bec4d8b71d8174baab30bc394962d63f72c9440 Mon Sep 17 00:00:00 2001 From: Michael Goin Date: Fri, 7 Aug 2020 13:06:14 -0700 Subject: [PATCH 3/3] refactor: cleanup on HttpTraceContext --- .../src/context/propagation/HttpTraceContext.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts b/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts index 3a46f26226..c5fe4a8e7d 100644 --- a/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts +++ b/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts @@ -29,6 +29,7 @@ export const TRACE_PARENT_HEADER = 'traceparent'; export const TRACE_STATE_HEADER = 'tracestate'; const VERSION = '00'; +const VERSION_PART_COUNT = 4; // Version 00 only allows the specific 4 fields. const VERSION_REGEX = /^(?!ff)[\da-f]{2}$/; const TRACE_ID_REGEX = /^(?![0]{32})[\da-f]{32}$/; @@ -49,9 +50,12 @@ export function parseTraceParent(traceParent: string): SpanContext | null { const trimmed = traceParent.trim(); const traceParentParts = trimmed.split('-'); - // Version 00 only allows the specific 4 fields. - // For future versions, we can grab just the parts (4 fields) we support. - if (traceParentParts[0] === VERSION && traceParentParts.length !== 4) { + // Current version must be structured correctly. + // For future versions, we can grab just the parts we do support. + if ( + traceParentParts[0] === VERSION && + traceParentParts.length !== VERSION_PART_COUNT + ) { return null; }