Pass W3C Trace Context test suite at strictness 1 #1406

michaelgoin · 2020-08-07T20:10:22Z

Which problem is this PR solving?

This fixes STRICT_LEVEL=1 failures surfaced by the test server PR: #1380.

No longer allows extra parts for version 00 of trace parent.
This is one of those implicit requirements that is not clearly stated out in the spec. Extra parts should only be allowed for future versions (grabbing what we support) but version 00 only has the 4 parts.
Tracestate Header list members now handle/strip optional whitespace (OWS).

W3C Trace Context spec: https://www.w3.org/TR/trace-context/
Validation test repo: https://github.com/w3c/trace-context/tree/master/test

Short description of the changes

Instead of a single-catch regex attempting to validate all potential versions, split into validators for each part.
Start by splitting and verifying correct # of parts for version before moving on.
Trim list members from trace state prior to grabbing key/value pairs

Prior to fix

======================================================================
ERROR: test_tracestate_multiple_headers_different_keys (__main__.TraceContextTest)
harness sends a request with multiple tracestate headers, each contains different set of keys
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test.py", line 531, in test_tracestate_multiple_headers_different_keys
    self.assertTrue(str(tracestate).index('bar=2') < str(tracestate).index('rojo=1'))
ValueError: substring not found

======================================================================
ERROR: test_tracestate_ows_handling (__main__.TraceContextTest)
harness sends a request with a valid tracestate header with OWS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test.py", line 612, in test_tracestate_ows_handling
    self.assertEqual(tracestate['foo'], '1')
  File "/Users/mgoin/Documents/development/srjames90/opentelemetry-js/integration-tests/target/trace-context/test/tracecontext/tracestate.py", line 32, in __getitem__
    return self._traits[key]
KeyError: 'foo'

======================================================================
FAIL: test_traceparent_version_0x00 (__main__.TraceContextTest)
harness sends an invalid traceparent with extra trailing characters
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test.py", line 194, in test_traceparent_version_0x00
    self.assertNotEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')
AssertionError: '12345678901234567890123456789012' == '12345678901234567890123456789012'

----------------------------------------------------------------------
Ran 40 tests in 0.608s

FAILED (failures=1, errors=3, skipped=6)

After Fix

----------------------------------------------------------------------
Ran 40 tests in 0.609s

OK (skipped=6)

michaelgoin · 2020-08-07T20:12:35Z

packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts

@@ -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})(-|$)/;


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.

michaelgoin · 2020-08-07T20:12:58Z

packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts

  if (
-    !match ||
-    match[1] === '00000000000000000000000000000000' ||


These validations moved into the respective regex validators.

michaelgoin · 2020-08-07T20:13:52Z

packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts

@@ -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();


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.

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

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.

michaelgoin · 2020-08-07T20:14:51Z

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<string, string>, part: string) => {
-        const i = part.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER);
+        const listMember = part.trim(); // Optional Whitespace (OWS) handling


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.

michaelgoin · 2020-08-07T20:15:14Z

packages/opentelemetry-core/test/context/HttpTraceContext.test.ts

+    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 ';


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

codecov · 2020-08-07T20:16:16Z

Codecov Report

Merging #1406 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   94.13%   94.15%   +0.01%     
==========================================
  Files         145      145              
  Lines        4314     4326      +12     
  Branches      877      881       +4     
==========================================
+ Hits         4061     4073      +12     
  Misses        253      253

Impacted Files	Coverage Δ
...y-core/src/context/propagation/HttpTraceContext.ts	`100.00% <100.00%> (ø)`
...ackages/opentelemetry-core/src/trace/TraceState.ts	`100.00% <100.00%> (ø)`

dyladan

LGTM thanks for taking this on

mwear

LGTM. Thanks @michaelgoin!

dyladan · 2020-08-12T15:03:43Z

@open-telemetry/javascript-approvers looking for a couple more approvals here. Now that we've merged the test suite, the build is failing every time until this is merged.

michaelgoin added 3 commits August 7, 2020 11:23

fix: fixed handling of extra traceparent fields for version 00

7741963

fix: fixes W3C tracestate OWS handling

18b4c5d

refactor: cleanup on HttpTraceContext

0bec4d8

michaelgoin requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners August 7, 2020 20:10

michaelgoin commented Aug 7, 2020

View reviewed changes

dyladan approved these changes Aug 8, 2020

View reviewed changes

dyladan added the bug Something isn't working label Aug 10, 2020

dyladan changed the title ~~W3C Trace Context Fixes~~ Pass W3C Trace Context test suite at strictness 1 Aug 10, 2020

mwear approved these changes Aug 10, 2020

View reviewed changes

dyladan added this to In progress PRs in GA Burndown via automation Aug 12, 2020

dyladan added the release:required-for-ga label Aug 12, 2020

GA Burndown automation moved this from In progress PRs to Approved Aug 13, 2020

obecny approved these changes Aug 13, 2020

View reviewed changes

markwolff approved these changes Aug 13, 2020

View reviewed changes

dyladan added 2 commits August 13, 2020 12:56

Merge branch 'master' into w3c-trace-context-strict1-fixes

a55c692

Merge branch 'master' into w3c-trace-context-strict1-fixes

31dc370

dyladan merged commit b368d32 into open-telemetry:master Aug 13, 2020

GA Burndown automation moved this from Approved to Done Aug 13, 2020

michaelgoin deleted the w3c-trace-context-strict1-fixes branch August 14, 2020 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass W3C Trace Context test suite at strictness 1 #1406

Pass W3C Trace Context test suite at strictness 1 #1406

michaelgoin commented Aug 7, 2020 •

edited

michaelgoin Aug 7, 2020

michaelgoin Aug 7, 2020

mayurkale22 Aug 10, 2020

michaelgoin Aug 7, 2020

mayurkale22 Aug 10, 2020

michaelgoin Aug 13, 2020

michaelgoin Aug 7, 2020

michaelgoin Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

dyladan left a comment

mwear left a comment

dyladan commented Aug 12, 2020

Pass W3C Trace Context test suite at strictness 1 #1406

Pass W3C Trace Context test suite at strictness 1 #1406

Conversation

michaelgoin commented Aug 7, 2020 • edited

Which problem is this PR solving?

Short description of the changes

Prior to fix

After Fix

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2020 • edited

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

dyladan commented Aug 12, 2020

michaelgoin commented Aug 7, 2020 •

edited

codecov bot commented Aug 7, 2020 •

edited