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

Make all test cases in W3C trace-context test suite pass #1668

Merged
merged 14 commits into from
Jan 6, 2021

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Dec 22, 2020

Fixed errors and failures (marked as "Fixed in this PR" below) in W3C Trace Context tests of #1219. (Tracking individual test case would be easier after merging PR #1243, which parses the test run result and identifies individual test case status.)

Note on key format

There is an inconsistency in Trace Context v1 between the expressions and the description in the note below.

Expressions:

key = lcalpha 0*255( lcalpha / DIGIT / "_" / "-"/ "*" / "/" )
key = ( lcalpha / DIGIT ) 0*240( lcalpha / DIGIT / "_" / "-"/ "*" / "/" ) "@" lcalpha 0*13( lcalpha / DIGIT / "_" / "-"/ "*" / "/" )
lcalpha    = %x61-7A ; a-z

Note:

Note: Identifiers MUST begin with a lowercase letter or a digit, and can only contain lowercase letters (a-z), digits (0-9), underscores (_), dashes (-), asterisks (*), and forward slashes (/).

I think the first expression has a bug and the key should start with ( lcalpha / DIGIT ) instead of lcalpha to be consistent with the note of Trace Context v1 and the next version of specification being developed in w3c/trace-context repository.

The same note has been added in code as inline comment (Ctrl+F for "inconsistency").

Note on vendor format

Trace Context v1 which has W3C Recommendation status differs from the next version of specification being developed in w3c/trace-context repository regarding the "tenant@vendor" rules, which are covered by the following two test cases in the python test code.

Here I implemented the rules in accordance with Trace Context v1 as following to pass the python test case.

test_tracestate_key_illegal_vendor_format: The trace state is in such format tenant@vendor. Neither tenant and vendor can be empty. There can be at most one @. Otherwise remove all other list-members even if they are valid (in other words, remove all trace states).
test_tracestate_key_length_limit: The key can be up to 256 characters. Vendor up to 14. Tenant up to 241. Otherwise remove all trace states.

Explanation on version number

Regarding the failing two cases on version number and generating new trace_id (test_traceparent_version_0x00 and test_traceparent_version_0xff), the feature was working until a bug was introduced in this change.

Summary of the failures in Netcoreapp 2.1

Errors:

  • test_tracestate_duplicated_keys: Fixed in this PR.
  • test_tracestate_key_illegal_characters: Fixed in this PR.
  • test_tracestate_key_illegal_vendor_format: Fixed in this PR.
  • test_tracestate_key_length_limit: Fixed in this PR.
  • test_tracestate_member_count_limit: Fixed in this PR.
  • test_tracestate_multiple_headers_different_keys: Fixed in this PR.
  • test_tracestate_value_illegal_characters: Fixed in this PR.

Failures:

Summary of the failures in Netcoreapp 3.1

Errors:

  • test_tracestate_duplicated_keys: Fixed in this PR.
  • test_tracestate_key_illegal_characters: Fixed in this PR.
  • test_tracestate_key_illegal_vendor_format: Fixed in this PR.
  • test_tracestate_key_length_limit: Fixed in this PR.
  • test_tracestate_member_count_limit: Fixed in this PR.
  • test_tracestate_multiple_headers_different_keys: Fixed in this PR.
  • test_tracestate_value_illegal_characters: Fixed in this PR.

Failures:

Summary of the failures in Netcoreapp 5.0

Errors:

  • test_tracestate_key_illegal_vendor_format: Fixed in this PR.
  • test_tracestate_key_length_limit: Fixed in this PR.

Failures:

@xiang17 xiang17 requested a review from a team December 22, 2020 09:29
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #1668 (a25f323) into master (e24dccb) will increase coverage by 0.21%.
The diff coverage is 98.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1668      +/-   ##
==========================================
+ Coverage   81.90%   82.12%   +0.21%     
==========================================
  Files         245      245              
  Lines        6637     6706      +69     
==========================================
+ Hits         5436     5507      +71     
+ Misses       1201     1199       -2     
Impacted Files Coverage Δ
....Api/Context/Propagation/TraceContextPropagator.cs 87.64% <98.70%> (+11.40%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️

…_length_limit

(cherry picked from commit 5a2bd56c8f83b8e9094534084988992184f4fe7e)
(cherry picked from commit 4e23f9aec69c59affb27327e0a624b32a82317cc)
@xiang17 xiang17 changed the title Fixing errors in W3CTraceContextTests Make all test cases in W3C trace-context test suite pass Dec 29, 2020
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas cijothomas merged commit c188edb into open-telemetry:master Jan 6, 2021
@xiang17 xiang17 deleted the xiang17/FixingW3CTests branch January 7, 2021 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants