-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix(w3c trace context): do not pass down unknown flags. #417
Conversation
❌ @basti1302 the
📝 What should I do to fix it?All proposed commits should include a sign-off in their messages, ideally at the end. ❔ Why it is requiredThe Developer Certificate of Origin (DCO) is a lightweight way for contributors to certify that they wrote or otherwise have the right to submit the code they are contributing to the project. Here is the full text of the DCO, reformatted for readability:
Contributors sign-off that they adhere to these requirements by adding a
Git even has a
|
65678d6
to
43c7dd0
Compare
❌ @basti1302 the
📝 What should I do to fix it?All proposed commits should include a sign-off in their messages, ideally at the end. ❔ Why it is requiredThe Developer Certificate of Origin (DCO) is a lightweight way for contributors to certify that they wrote or otherwise have the right to submit the code they are contributing to the project. Here is the full text of the DCO, reformatted for readability:
Contributors sign-off that they adhere to these requirements by adding a
Git even has a
|
43c7dd0
to
5572d75
Compare
@basti1302 Rebasing onto the current |
65caa18
to
5c4d33b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great job.
@basti1302 this PR needs to rebase before merging it. |
5c4d33b
to
232aaa4
Compare
@pvital I just rebased again, should be good now. |
My apologies, #421 went in the meantime. |
@basti1302 if the PR is OK, we can rebase before merging it. Just say, and we can do it. |
The W3C trace context specification mandates that a participant must only send known flags downstream. See https://www.w3.org/TR/trace-context/#other-flags: "The behavior of other flags, such as (00000100) is not defined and is reserved for future use. Vendors MUST set those to zero." In particular, this commit changes the way traceparent.py handles the flags. Instead of persisting the complete flags field from traceparent, we specifically parse the flag(s) that we understand and keep them as individual boolean attributes. This will also make it easier to handle the random trace ID flag correctly when updating support to W3C trace context level 2. In addition we now correctly pass down the version field as 00, since that is the traceparent version we support (instead of passing down the incoming version value). Signed-off-by: Bastian Krol <bastian.krol@ibm.com>
232aaa4
to
df2a32c
Compare
Lolol, no merges since weeks and today you are merging all the PRs 🎉 No worries, I rebased again. But if you plan to merge more PRs before this one, you can also feel free to rebase this on your own. |
The W3C trace context specification mandates that a participant must only send known flags downstream. See
https://www.w3.org/TR/trace-context/#other-flags: "The behavior of other flags, such as (00000100) is not defined and is reserved for future use. Vendors MUST set those to zero."