open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
326 stars 157 forks source link

Update 'r' value generation based on randomness flag + editorial changes to improve clarity #261

Open kalyanaj opened 1 month ago

kalyanaj commented 1 month ago

Update 'r' value generation based on randomness flag + editorial changes to improve clarity.

tigrannajaryan commented 1 month ago

We don't typically update OTEPs with any substantial content. Minor editorial changes are fine, but otherwise normally a new OTEP is expected.

kalyanaj commented 1 month ago

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.

jmacd commented 1 month ago

Suggestion from Kent: please try to minimize diffs by keeping to one sentence per line of markdown.

jmacd commented 3 weeks ago

@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:

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!