open-telemetry
Update 'rv' value generation based on randomness flag + editorial changes to improve clarity
Update 'r' value generation based on randomness flag + editorial changes to improve clarity.
We don't typically update OTEPs with any substantial content. Minor editorial changes are fine, but otherwise normally a new OTEP is expected.
Hi @tigrannajaryan, to give you some context, while the diff looks bigger, there is no change to the proposal or approach itself, so from that perspective there are no changes (substantial or otherwise). This PR is an effort to improve the clarity and change the flow/wording (based on a discussion in the Sampling SIG) to improve the readability for future implementers of this spec.
Suggestion from Kent: please try to minimize diffs by keeping to one sentence per line of markdown.
@kalyanaj now that I've read this PR in detail, I agree with @tigrannajaryan's suggestion. This PR is difficult to review because of all the editorial changes, which I also appreciate. I think it would be easiest if we open a new PR with a completely new document, with a new OTEP number. Then, update the OTEP 235 document to refer to the new OTEP, along with an explanation of the non-editorial changes relative to 235.
In my words, the only change here is to say:
- root/head samplers must insert "rv" if random flag is not set
- child/head samplers must assume trace ID is random if "rv" is not set.
I see this change as a bug fix, but it is still a change. I think readers will be happy to read the edited document either way, but leaving OTEP 235 "as-is" may be easiest here. Thanks!
I have copied the bulk of this PR into https://github.com/open-telemetry/opentelemetry-specification/pull/4166.
We should merge this, just need approvals. The language in this PR is already incorporated into the spec PRs, no reason not to merge.
I wish to certify that this is 99% editorial change and the rest is to correct a problem discovered during early implementation. We believe there are no other implementations of OTEP 235 in the wild that have not already incorporated the changes in this PR. Please merge.
Hey @kalyanaj
Sorry for the bad timing, but we have moved our OTEPs to the Specification repo, so would you mind opening a PR against it? Specifically https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/trace/0235-sampling-threshold-in-trace-state.md
We have more relaxed approval requirements there so we should get it merged there super fast, as we will clarify this OTEP has enough approvals and has been extensively discussed.
OTEPs have been moved to the Specification repository. Please consider re-opening this PR against the new location. Closing.