Details attributes and http header encoding #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since I'm working on a change in Brave, I figured I'd go ahead and elaborate the common attributes here. Further clarifications can either be made in this PR, or be saved for later ones.
| * Debug trace: When setting Flags to 1, sampling is implicit | ||
| * Externally provisioned IDs: When you want to control IDs, but not sampling policy | ||
|
|
||
| Unless it is a debug trace, leaving sampled unset is typically for ID correlation. For example, someone re-uses a global identifier from another system, or correlating in logs. In these cases, the caller knows the ID they want, but allows the next hop to decide if it will be traced or not. The caller should not report a span to the tracing system using this ID unless they propagate Sampled=1. |
There was a problem hiding this comment.
cc @jcarres-mdsol @dangets This particularly relates to functionality in Finagle, which is missing in Brave openzipkin/brave#277
shakuzen
approved these changes
Nov 23, 2016
|
|
||
| ## Sampled | ||
|
|
||
| When the Sampled is 1, report this span to the tracing system. When it is 0, do do not. When B3 attributes are sent without the Sampled attribute, the receiver should make the decision. Once Sampled is set to 0 or 1, the value should be consistently sent downstream. |
There was a problem hiding this comment.
"When it is 0, do do not." → "When it is 0, do not."
|
|
||
| ## Sampled | ||
|
|
||
| When the Sampled is 1, report this span to the tracing system. When it is 0, do do not. When B3 attributes are sent without the Sampled attribute, the receiver should make the decision. Once Sampled is set to 0 or 1, the value should be consistently sent downstream. |
There was a problem hiding this comment.
"When the Sampled is 1" sounds strange to me. Maybe "When Sampled is 1" or "When the Sampled attribute is 1"
There was a problem hiding this comment.
Thanks.. I think I meant When the Sampled flag is 1
codefromthecrypt
pushed a commit
to openzipkin/brave
that referenced
this pull request
Nov 23, 2016
There are some who want to control trace identifiers, but leave the sampling decision to the next hop. In this case, they propagate the required headers (X-B3-TraceId, X-B3-ParentId) and not X-B3-Sampled. When this happens, the receiver of the headers (the server), should sample the trace ID before using it. Before, we weren't implementing this (eventhough Finagle does). The change here addresses it and also backfills tests accordingly. See https://github.com/twitter/finagle/blob/develop/finagle-http/src/main/scala/com/twitter/finagle/http/Codec.scala#L341 See https://github.com/twitter/finagle/blob/develop/finagle-core/src/main/scala/com/twitter/finagle/tracing/Tracer.scala#L119 See openzipkin/b3-propagation#8 Fixes #277
|
I'm going to merge this in the spirit of getting something far better than the template README out there. Will take any more comments after commit, so please voice if you have them! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since I'm working on a change in Brave, I figured I'd go ahead and elaborate the common attributes here. Further clarifications can either be made in this PR, or be saved for later ones.