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

two traceparent headers are not rejected #1676

Closed
malafeev opened this issue Sep 22, 2020 · 9 comments
Closed

two traceparent headers are not rejected #1676

malafeev opened this issue Sep 22, 2020 · 9 comments
Assignees
Labels
Bug Something isn't working help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release

Comments

@malafeev
Copy link
Contributor

Next w3c context propagation tests fails:

def test_traceparent_duplicated(self):
		'''
		harness sends a request with two traceparent headers
		expects a valid traceparent from the output header, with a newly generated trace_id
		'''
		traceparent, tracestate = self.make_single_request_and_get_tracecontext([
			['traceparent', '00-12345678901234567890123456789011-1234567890123456-01'],
			['traceparent', '00-12345678901234567890123456789012-1234567890123456-01'],
		])
		self.assertNotEqual(traceparent.trace_id.hex(), '12345678901234567890123456789011')
		self.assertNotEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')

Got in #1653
Can be reproduced by running tests from mentioned PR

@malafeev malafeev added the Bug Something isn't working label Sep 22, 2020
@jkwatson jkwatson added help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release labels Sep 22, 2020
@malafeev
Copy link
Contributor Author

not a bug, it's an issue of an extractor.
Extractor should check that there are more than one header with the same key.
Some frameworks can return first header.

@iNikem
Copy link
Contributor

iNikem commented Sep 23, 2020

Wait, we still need to fix "an issue of an extractor", no?

@Oberon00
Copy link
Member

We need to fix an issue of the getter API first if we want to fix this in the extractor open-telemetry/opentelemetry-specification#433

@malafeev
Copy link
Contributor Author

custom extractor can check multi-value header and return joined string instead of first found value.

@jkwatson
Copy link
Contributor

This is still a bug in the overall handling of the W3C headers, though, isn't it, @malafeev ? I don't think this should be closed until it's working to spec, no matter where the issue lies.

@jkwatson jkwatson reopened this Sep 23, 2020
@malafeev
Copy link
Contributor Author

ok

@jkwatson jkwatson self-assigned this Oct 19, 2020
@jkwatson
Copy link
Contributor

Note: you have to run at STRICT=2 to see this error (and a couple of others)

@jkwatson
Copy link
Contributor

@malafeev This test passes for me on the main branch, and I see the extractor concatenates the headers with a comma between them.

Now that I have dug into the issue, I understand more of what you were saying. I think that this bug should be closed, because our W3C TraceContext implementation works to spec (in this instance at least). If someone's extractor is throwing data away, our HttpTraceContext implementation shouldn't be held to account.

If we need to change the extractor/getter interface to allow more than one header, then that's a totally separate issue, and not this one. :)

@jkwatson
Copy link
Contributor

Closing, as not-a-bug. Sorry for the confusion, @malafeev !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release
Projects
None yet
Development

No branches or pull requests

4 participants