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

Misleading statements in the blog post #1790

Closed
yurishkuro opened this issue Sep 28, 2022 · 1 comment
Closed

Misleading statements in the blog post #1790

yurishkuro opened this issue Sep 28, 2022 · 1 comment
Labels
blog bug Something isn't working

Comments

@yurishkuro
Copy link
Member

Hey folks. This is regarding #1721.

Nice blog post. While this does not change the gist of the post, I found your explanation of the actual bug rather confusing.

First, the comparison of tracestate as multiple headers vs. traceparent is misleading. tracestate is a comma-separated list of entries, and since that's how RFC7230 defines header concatenation, it works as you described. But traceparent is not a list, it's a single unbreakable string, so the reasoning for header concatenation does not apply. Also your reasoning about Java vs. other SDKs seems backwards - one would argue it was Java SDK that was non-compliant with Trace-Context spec because it was supposed to fail to parse clearly malformed traceparent, not just take the first value.

Second, the screenshot shows that your traceparent has a list of a whole number of entries, each representing completely different traces. Where were they all coming from? Obviously not from the caller, the caller would only send one trace-id (unless the caller instrumentation was broken). I am guessing the issue may have been some static storage in nginx module where it was accumulating and adding all previously seen traceparent entries to each outbound request, instead of the single entry coming in the inbound request, but the discussion of the blog talks about different things.

Since this is a blog post on the official OTEL website, could you please correct these mis-statements?

cc @svrnm @sanketmehta28 @kpratyus

@yurishkuro yurishkuro added bug Something isn't working blog labels Sep 28, 2022
@svrnm
Copy link
Member

svrnm commented Sep 29, 2022

hey @yurishkuro,

I wrote most of these sentences, so I take full responsibility ;-) ... and I am happy to get this fixed with your help!

Here's what we have as of today:

There it is! The trace headers (baggage, traceparent, tracestate) are send as multiple header fields.
Doing some research on that topic, we found out, that this is coved by RFC7230 and is allowed and defined for tracestate, but the definition for traceparent does not provide details on that corner-case.

I anticipate that you're one of the editors of the W3C Trace Context document, so read the following as the thought process from a layman that is reading that document & the RFC7230 and then trying to write down those sentences above.

What I wanted to say here is that I understood, that RFC7230 defines header concatenation as a comma-separated list of entries for all headers that might be set. In the description of tracestate I read that this is OK (Tracestate MAY be sent or received as multiple header fields), whereas traceparent does not explicitly state that this is not OK (and that's why I wrote that it "does not provide details on that corner-case").

Let me try to formulate this more accurately.

Although we didn’t read into the source code of the OTel Java SDK & OTel Python SDK, it was obvious that Java was flexible in taking traceparent as multiple header field, while python (and other languages) do not allow that.

I didn't intend to say here that python (and others) should change and Java is doing it properly, all I wanted to call out is the difference. In our specific case Java (silently & maybe non-compliant) did what we expected and python didn't (also silently & maybe compliant).

I can add a sentence that clarifies that.

To fix our problem we added some checks to the nginx module, that make sure that the trace headers are only set once.

This is where the list of multiple traceparent entries was coming from, it was a bug in the nginx module.

We need to make that clearer in the analysis section: the root cause of the problem was within the nginx module.

svrnm added a commit to svrnm/opentelemetry.io that referenced this issue Oct 12, 2022
Signed-off-by: svrnm <neumanns@cisco.com>
@svrnm svrnm closed this as completed in ecf2136 Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants