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
docs: add trace observation point description #23028
Conversation
63b1ccf
to
1a9f0cd
Compare
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.
Thanks a lot, this is going to be very helpful. I've left two minor comments
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.
Thanks!
/test |
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.
I found a couple typos below, but otherwise looks good to me. Thanks!
Please squash all commits into the first. |
41ada2b
to
c58a366
Compare
/test |
|
Marking this ready-to-merge. This PR only updates protobuf schema comments, which are validated by performing a |
I've removed the ready-to-merge label based on (what I think is good) out-of-band feedback by @joestringer. He noticed that the language is a bit inconsistent - there are three different sets of wording being used for different locations, eg "originated", "coming into", "passed from". However, there are only two broad actions: Transmit (TO_) and Receive (FROM_) - thus the suggestion to make the wording a bit more consistent and reduce the set of verbs. That's me paraphrasing him - I'll let @joestringer chime in as well if he wants. @mainred Do you think you could improve the language a bit to make sure we're using more consistent verbs (i.e. only use transmit or receive)? In terms of the used nouns, the usage seems already pretty consistent to me. The only asymmetry I've found is regarding the tunnel device, from-overlay mentions |
c444cb6
to
318a9fa
Compare
@gandro Thanks for being thoughtful. I totally agree with you, as this is also customer-facing, using consistent phrases can bring less confusion to understanding different obs points. Updated the code to use "are received by" and "are transmitted from" |
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.
Thanks, this is a lot more consistent and IMHO easier to reason about.
There's one more subtle distinction that I'd like to make if we are going to define the semantics like this. See the comments below. This applies to all of the cases, TO_*
-> transmit, FROM_*
-> receive.
Signed-off-by: Qingchuan Hao <qingchuan.hao@microsoft.com>
318a9fa
to
ac38b67
Compare
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.
LGTM thanks.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #issue-number