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

Pass W3C Trace Context test suite at strictness 1 #1406

Merged

Conversation

michaelgoin
Copy link
Contributor

@michaelgoin michaelgoin commented Aug 7, 2020

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)

@@ -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.

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

@@ -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.

@@ -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.

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.

@codecov
Copy link

codecov bot commented Aug 7, 2020

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%> (ø)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for taking this on

@dyladan dyladan added the bug Something isn't working label Aug 10, 2020
@dyladan dyladan changed the title W3C Trace Context Fixes Pass W3C Trace Context test suite at strictness 1 Aug 10, 2020
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @michaelgoin!

@dyladan
Copy link
Member

dyladan commented Aug 12, 2020

@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.

@dyladan dyladan merged commit b368d32 into open-telemetry:master Aug 13, 2020
@michaelgoin michaelgoin deleted the w3c-trace-context-strict1-fixes branch August 14, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants