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.

Sign up for GitHub

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

Jump to bottom

Pass W3C Trace Context test suite at strictness 1 #1406

Merged
merged 5 commits into from
Aug 13, 2020
+58 −11
Merged

Pass W3C Trace Context test suite at strictness 1 #1406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7741963
fix: fixed handling of extra traceparent fields for version 00
michaelgoin Aug 7, 2020
18b4c5d
fix: fixes W3C tracestate OWS handling
michaelgoin Aug 7, 2020
0bec4d8
refactor: cleanup on HttpTraceContext
michaelgoin Aug 7, 2020
a55c692
Merge branch 'master' into w3c-trace-context-strict1-fixes
dyladan Aug 13, 2020
31dc370
Merge branch 'master' into w3c-trace-context-strict1-fixes
dyladan Aug 13, 2020
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
36 changes: 28 additions & 8 deletions packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +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})(-|$)/;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a single regex can be nice aesthetically, it became kinda awkward here trying to support current (strict) version requirements and future (less-strict) requirements.

Could have handled by also capturing the version and doing a later comparison of version + parsed final match part (-|$) but that seemed less straightforward and likely to need overhaul in future versions. I'm hoping the different parts makes any potential future version require less restructuring.


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}$/;
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}
Expand All @@ -41,19 +47,33 @@ 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);
const trimmed = traceParent.trim();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguable if we need this. Don't believe there's anything in the spec saying we need to support such but also seems nice to have.

That being said, it does add an additional string creation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it superfluous? I don't think this is being done in other languages. Would be nice to know the reasoning behind this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, there's a chance you could have a trailing whitespace character (OWS) in the traceparent and this removes that, so as not to lump it into the flags validation. It is just a potential nice to have.

It does look like trailing whitespace values are removed as of Node 10.19.0, 12.15.0 and 13.8.0 for security reasons (all released on 02/06/2020), so it will be wasted effort as of those versions. I'm assuming we'll drop Node 8 support at some point to match Node itself, but that will continue to have issues.

Happy to remove in this PR, or a follow-up, if we don't want that code in there.

const traceParentParts = trimmed.split('-');

// Current version must be structured correctly.
// For future versions, we can grab just the parts we do support.
if (
!match ||
match[1] === '00000000000000000000000000000000' ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These validations moved into the respective regex validators.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

match[2] === '0000000000000000'
traceParentParts[0] === VERSION &&
traceParentParts.length !== VERSION_PART_COUNT
) {
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),
};
}

Expand Down
7 changes: 4 additions & 3 deletions packages/opentelemetry-core/src/trace/TraceState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>, part: string) => {
const i = part.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER);
const listMember = part.trim(); // Optional Whitespace (OWS) handling
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds the OWS support / fixes those related errors. While it does add an additional string, there's likely other more impactful optimizations we could do in this area if it became a concern.

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 {
Expand Down
26 changes: 26 additions & 0 deletions packages/opentelemetry-core/test/context/HttpTraceContext.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -245,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 ';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable subset of what we were failing on.

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,
Expand Down