Add the random trace id flag #474

dyladan · 2021-11-09T18:15:58Z

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

spec/60-trace-id-format.md

yurishkuro · 2021-11-09T18:23:57Z

If this resolves #467, please state this in the description.

Does this not require a bump to the header's version number?

spec/20-http_request_header_format.md

bogdandrutu · 2021-11-09T18:30:00Z

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.

dyladan · 2021-11-09T19:58:57Z

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.

spec/60-trace-id-format.md

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

basti1302

LGTM, I left one nitpick/phrasing suggestion.

(And I assume the two open TODOs need to be decided and removed before merging).

spec/20-http_request_header_format.md

yurishkuro · 2021-11-16T13:11:43Z

Note that while this resolves #467, it does not address #463 because it makes the operator choose between using externally generated ids (sometimes needed for legacy/compatibility reasons) and having randomness in the context.

dyladan · 2021-11-16T17:35:34Z

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.

yurishkuro · 2021-11-16T18:24:19Z

It addresses part of #463 which is that the trace id can be used to ensure sampling is consistent between services.

It "addresses #463" only by introducing another limitation.

dyladan · 2021-11-16T18:34:32Z

I'm afraid we might be talking past each other. I'm not entirely sure I understand what you're arguing.

yurishkuro · 2021-11-16T18:43:33Z

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.

dyladan · 2021-11-16T18:49:11Z

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.

yurishkuro · 2021-11-16T18:53:35Z

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).

dyladan · 2021-11-16T18:56:54Z

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

yurishkuro · 2021-11-16T19:06:49Z

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.

dyladan · 2022-04-12T15:15:33Z

I have added the SHOULD wording and some wording about why users should be motivated to use the flag. Please take a look and review when you can.

spec/20-http_request_header_format.md

spec/60-trace-id-format.md

spec/20-http_request_header_format.md

yurishkuro · 2022-04-12T16:00:25Z

spec/20-http_request_header_format.md

@@ -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`.


Do we not need a new version if we're adding a new flag?

It is backwards compatible so it is not required. We can decide to bump the version if we feel it is different enough.

spec/20-http_request_header_format.md

SergeyKanzhelev

a few suggestions added

* 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>

Add the random trace id flag

08f557b

yurishkuro reviewed Nov 9, 2021

View reviewed changes

spec/60-trace-id-format.md Outdated Show resolved Hide resolved

bogdandrutu reviewed Nov 9, 2021

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

dyladan force-pushed the random-flag branch 2 times, most recently from ed11809 to 65b2552 Compare November 9, 2021 20:00

Wording

b32f18a

dyladan force-pushed the random-flag branch from 65b2552 to b32f18a Compare November 9, 2021 20:00

yurishkuro approved these changes Nov 9, 2021

View reviewed changes

spec/60-trace-id-format.md Outdated Show resolved Hide resolved

Wording

f65d9a5

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

basti1302 approved these changes Nov 11, 2021

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

SergeyKanzhelev mentioned this pull request Nov 16, 2021

An alternative proposal on implementing randomness by updating level … #480

Closed

dyladan and others added 3 commits April 11, 2022 16:42

Merge remote-tracking branch 'origin/main' into random-flag

ef9e4ff

Add SHOULD wording to trace id randomness

8617fef

Specify that implementers SHOULD set random flag when appropriate

6206ae8

kalyanaj reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Show resolved Hide resolved

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

spec/60-trace-id-format.md Outdated Show resolved Hide resolved

dyladan added 2 commits April 12, 2022 11:41

Random flag means at least 7 bytes

3eb81c2

Flag wording

05f145f

Only 2 flags are specified

971d081

yurishkuro reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 12, 2022

View reviewed changes

kalyanaj reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

dyladan added 2 commits April 12, 2022 14:08

Remove redundant wording

032f220

At least 7 bytes

41744ee

kalyanaj approved these changes Apr 12, 2022

View reviewed changes

SergeyKanzhelev reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Apr 12, 2022

View reviewed changes

spec/20-http_request_header_format.md Show resolved Hide resolved

SergeyKanzhelev approved these changes Apr 12, 2022

View reviewed changes

dyladan added 2 commits April 13, 2022 08:28

Review comments

7985b6a

Remove RECOMMENDED wording

2276524

SergeyKanzhelev approved these changes Apr 13, 2022

View reviewed changes

kalyanaj approved these changes Apr 13, 2022

View reviewed changes

kalyanaj merged commit cd1c099 into w3c:main Apr 13, 2022

dyladan mentioned this pull request Apr 26, 2022

cherry-pick: Add the random trace id flag (#474) #486

Merged

jmacd mentioned this pull request Sep 14, 2022

Review approach & specify algorithm for TraceIdRatioBasedSampler (ProbabilitySampler) open-telemetry/opentelemetry-specification#1413

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the random trace id flag #474

Add the random trace id flag #474

dyladan commented Nov 9, 2021 •

edited by pr-preview bot

Loading

yurishkuro commented Nov 9, 2021

bogdandrutu commented Nov 9, 2021

dyladan commented Nov 9, 2021

basti1302 left a comment

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021 •

edited

Loading

yurishkuro commented Nov 16, 2021

dyladan commented Apr 12, 2022

yurishkuro Apr 12, 2022

dyladan Apr 12, 2022

SergeyKanzhelev left a comment

		@@ -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`.

Add the random trace id flag #474

Add the random trace id flag #474

Conversation

dyladan commented Nov 9, 2021 • edited by pr-preview bot Loading

yurishkuro commented Nov 9, 2021

bogdandrutu commented Nov 9, 2021

dyladan commented Nov 9, 2021

basti1302 left a comment

Choose a reason for hiding this comment

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021

yurishkuro commented Nov 16, 2021

dyladan commented Nov 16, 2021 • edited Loading

yurishkuro commented Nov 16, 2021

dyladan commented Apr 12, 2022

yurishkuro Apr 12, 2022

Choose a reason for hiding this comment

dyladan Apr 12, 2022

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

dyladan commented Nov 9, 2021 •

edited by pr-preview bot

Loading

dyladan commented Nov 16, 2021 •

edited

Loading