Add the random trace id flag #474
Conversation
|
If this resolves #467, please state this in the description. Does this not require a bump to the header's version number? |
No, since this adds just a flag to the trace-flags and is backwards compatible. |
I agree with Bogdan here. |
dyladan
force-pushed
the
random-flag
branch
2 times, most recently
from
ed11809
to
65b2552
Compare
|
It addresses part of #463 which is that the trace id can be used to ensure sampling is consistent between services. It does not address the part about propagating the probability itself which is intentional. |
|
I'm afraid we might be talking past each other. I'm not entirely sure I understand what you're arguing. |
|
Sorry, let me try again. #463 presents a high level problem - we want to have access to a single random number across all nodes in the trace. This PR says you can have it but ONLY if your trace-id is random(-ish). So this PR only solves the high level problem for a subset of systems that can also meet the additional restriction of random IDs. In contrast, the proposal in #463 solves the problem for everyone. |
OK I see what you're saying, but in order to implement #463 you would still need a random number at the head of the trace. If the restriction is because of the random requirement, why does it not affect the other proposal? Sorry if this is obvious I just want to make sure I fully understand. |
|
I don't think the need to generate a random number is a blocker, having a random trace-id is. There are situations where traces must reuse a 3rd-party identifier as trace-id on which no randomness guarantees can be made (same reason why traceparent spec does not require random trace-id, we only use SHOULD). |
|
Thanks for the explanation. Would you rather have the proposal in #463 (or something like it)? We discussed it at a meeting and even started making a draft prototype #468 but some people thought it was too breaking of a change. edit: it's obviously possible to have both, but with #463/#468 there is less need for this |
|
I am not opposed to the new bit proposal, I like it, we just need to be aware that it does not solve the problem for everyone. |
|
I have added the |
| @@ -116,6 +120,8 @@ Vendors MUST ignore the `traceparent` when the `parent-id` is invalid (for examp | |||
|
|
|||
| #### trace-flags | |||
|
|
|||
| The current version of this specification (`00`) supports only two flags: `sampled` and `random-trace-id`. | |||
There was a problem hiding this comment.
Do we not need a new version if we're adding a new flag?
There was a problem hiding this comment.
It is backwards compatible so it is not required. We can decide to bump the version if we feel it is different enough.
* Add the random trace id flag * Wording * Wording Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * Add SHOULD wording to trace id randomness * Specify that implementers SHOULD set random flag when appropriate * Random flag means at least 7 bytes * Flag wording * Only 2 flags are specified * Remove redundant wording * At least 7 bytes * Review comments * Remove RECOMMENDED wording Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
* Add the random trace id flag * Wording * Wording Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * Add SHOULD wording to trace id randomness * Specify that implementers SHOULD set random flag when appropriate * Random flag means at least 7 bytes * Flag wording * Only 2 flags are specified * Remove redundant wording * At least 7 bytes * Review comments * Remove RECOMMENDED wording Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Resolves #467
As discussed in the previous working group meeting, this PR adds a trace flag to signal to downstream services that part of the trace id is random. Randomness in the trace ID is desirable because it may be used for things like consistent trace sampling, sharding, and more.
/cc @bogdandrutu author of #467
/cc @jmacd @oertl
Preview | Diff
Preview | Diff