Skip to content
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

Editorial change from 'recorded' flag to 'sampled' flag. #301

Closed
wants to merge 0 commits into from
Closed

Editorial change from 'recorded' flag to 'sampled' flag. #301

wants to merge 0 commits into from

Conversation

danielkhan
Copy link
Contributor

@danielkhan danielkhan commented May 20, 2019

This addresses #265
The 'recorded' wording when not referring to the flag itself was deliberately left untouched.
Another PR will change the paragraph to be more clear and concise.

@danielkhan danielkhan added the Editorial The reported issue can be addressed with an editorial change. This tag could be combined with others label May 20, 2019
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some changes unrelated to PR broke the spec. I think it's some markdown tool you use.

In text I feel there is now a mix or recorded indicating recording and sampled indicating decision. I tried to review where it make sense to keep and where to replace to sampled. I'd advice to re-read again to make sure nothing got left out


When set, the least significant bit documents that the caller may have recorded
When set, the least significant bit documents that the caller may have sampled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one problem with sampled - people not closely familiar with the subject have trouble interpreting "sampled = sampled in" or "sampled = sampled out". Recorded didn't have this problem. Perhaps we can keep the word "recorded" here or specify that sampled means recorded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

subset of requests.
2. Component may also fall back to probability sampling to set flag
`recorded` to `1` for the subset of requests.
1. Component that makes deferred or delayed recording decision may
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you broke the sublist. Please make sure to validate your changes using Editing the spec instructions:

image

spec/20-http_header_format.md Outdated Show resolved Hide resolved
@@ -224,24 +224,24 @@ for future use. Implementations MUST set those to zero.

### Examples of HTTP headers

*Valid traceparent when caller recorded this request:*
_Valid traceparent when caller recorded this request:_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this change?

spec/20-http_header_format.md Outdated Show resolved Hide resolved
spec/30-processing-model.md Outdated Show resolved Hide resolved
@@ -312,9 +312,10 @@ the `OWS` rule from <a data-cite='!RFC7230#section-3.2.3'>RFC7230 section

The `OWS` rule defines an optional whitespace. It is used where zero or more
whitespace characters might appear. When it is preferred to improve readability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was not intended to be a list =). It is a dash as a punctuation. Please remove newline.

@danielkhan danielkhan closed this May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editorial The reported issue can be addressed with an editorial change. This tag could be combined with others
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants