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

Allow digit as first chr in vendor specific trace state key #511

Merged

Conversation

mariojonke
Copy link
Contributor

Allows vendor specific trace state keys to start with a digit as specified in: https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#key

@mariojonke mariojonke requested a review from a team March 19, 2020 13:20
@codecov-io
Copy link

codecov-io commented Mar 19, 2020

Codecov Report

Merging #511 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #511   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files          43       43           
  Lines        2213     2213           
  Branches      250      250           
=======================================
  Hits         1980     1980           
  Misses        161      161           
  Partials       72       72           
Impacted Files Coverage Δ
...ry/trace/propagation/tracecontexthttptextformat.py 84.28% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12b4d6a...f0bd325. Read the comment docs.

@mauriciovasquezbernal
Copy link
Member

Hi @mariojonke,

Shouldn't it be modified as well?

_KEY_WITHOUT_VENDOR_FORMAT = r"[a-z][_0-9a-z\-\*\/]{0,255}"

Ideally a change like it should include a test to ensure we don't introduce a regression in the future. Maybe you want to create / modify one in https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/tests/trace/propagation/test_tracecontexthttptextformat.py.

@mariojonke
Copy link
Contributor Author

Hi @mauriciovasquezbernal

i think the _KEY_WITHOUT_VENDOR_FORMAT is in line with the ABNF in https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#key

Regarding tests, i'll add some.

@mauriciovasquezbernal
Copy link
Member

@mariojonke you're right. Probably what is not aligned is the text there "Identifiers MUST begin with a lowercase letter or a digit".

@@ -222,3 +222,32 @@ def test_tracestate_header_with_trailing_comma(self):
)
)
self.assertEqual(span.get_context().trace_state["foo"], "1")

def test_tracestate_keys(self):
"""Do not propagate invalid trace context.

Choose a reason for hiding this comment

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

Is this sentence the wrong one for this test?

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

Besides the small nit on the tests docstring, LGTM!

@Oberon00
Copy link
Member

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Great, thanks for the catch!

@toumorokoshi toumorokoshi merged commit 929adcd into open-telemetry:master Mar 23, 2020
@mariojonke mariojonke deleted the tracecontext-key-validation branch March 27, 2020 06:23
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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.

5 participants