-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update 'rv' value generation based on randomness flag + editorial changes to improve clarity #261
base: main
Are you sure you want to change the base?
Update 'rv' value generation based on randomness flag + editorial changes to improve clarity #261
Conversation
…yanaj/oteps into kalyanaj-235editorialchanges
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:
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 open-telemetry/opentelemetry-specification#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. |
@@ -80,23 +102,54 @@ The `T` value MUST be derived as follows: | |||
|
|||
Sampling Decisions MUST be propagated by setting the value of the `th` key in the Tracestate header according to the above. | |||
|
|||
## Initializing and updating T and R values | |||
This design requires that as a given span progresses along its collection path, `th` is non decreasing (and, in particular, must be increased at stages that apply lower sampling probabilities). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph and the next one seem to imply that increasing T/th is fine, but decreasing it is not, whereas the Head Samplers sections puts this as an actual example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @kalyanaj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Carlos,
The main difference is whether this is for the same span or a different span:
- For a particular span S1, this spec says that its 'th' value in the context should not be decreased as it flows through the pipeline (even if additional downstream samplers are in the flow).
- But a different span S2's (say a child span of S1) initial 'th' value can be anything -- based on the sampling rate of that operation -- this is the example used in the Head Samplers section.
Please let me know if this clarifies what you are looking for. We also have this text in this PR that tries to clarify this point:
It does not, however, restrict a span's initial th in any way. If a parent-based consistent sampler is used, a span's initial th would be the same as its parent's th value, else it would be a new value based on the sampling rate chosen for that span. In other words, the sampling rate for each operation can be chosen independently, and this would map to having different th values for different spans. But for any particular span, it is not acceptable for a downstream sampler to decrease the th value in its context.
--
Also adding @kentquirk who is the original author of this spec.
Update 'r' value generation based on randomness flag + editorial changes to improve clarity.