-
Notifications
You must be signed in to change notification settings - Fork 897
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
Change Status to be consistent with Link and Event #1067
Conversation
Signed-off-by: Bogdan Drutu <[email protected]>
Signed-off-by: Bogdan Drutu <[email protected]>
Sets the [`Status`](#status) of the `Span`. If used, this will override the | ||
default `Span` status, which is `OK`. | ||
Sets the `Status` of the `Span`. If used, this will override the default `Span` | ||
status, which is `Unset`. |
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 is fixing a bug in the specs (I guess)
Signed-off-by: Bogdan Drutu <[email protected]>
Application developers and Operators may set the status code to `Ok`. | ||
|
||
Analysis tools SHOULD respond to an `Ok` status by suppressing any errors they | ||
would otherwise generate. For example, to suppress noisy errors such as 404s. |
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.
Not following - why would analysis tools generate errors?
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.
Good question, I was not changing this, will file the issue #1068 to track this.
specification/trace/api.md
Outdated
to ignore previous calls. | ||
`Status` is semantically defined by the following properties: | ||
|
||
- `StatusCanonicalCode` represents the canonical set of `Status` codes. |
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.
Why are we using such a long name as StatusCanonicalCode? Are there non-canonical status codes?
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.
Good question, I was not changing this, will file the issue #1069 to track this.
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Bogdan Drutu <[email protected]>
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, from what I see it is primarily editorial changes and content reshulfling.
Only the value of the last call will be recorded, and implementations are free | ||
to ignore previous calls. |
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 is only taken over but could be refined to be worded consistently (MUST/SHOULD/MAY) either here or in a follow-up.
Co-authored-by: Armin Ruech <[email protected]>
* Change Status to be consistent with Link and Event Signed-off-by: Bogdan Drutu <[email protected]> * Add changelog entry Signed-off-by: Bogdan Drutu <[email protected]> * Fix link to status Signed-off-by: Bogdan Drutu <[email protected]> * Update specification/trace/api.md Co-authored-by: Yuri Shkuro <[email protected]> * Update specification/trace/api.md Co-authored-by: Yuri Shkuro <[email protected]> * Fix markdown errors Signed-off-by: Bogdan Drutu <[email protected]> * Update specification/trace/api.md Co-authored-by: Armin Ruech <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Armin Ruech <[email protected]>
Signed-off-by: Bogdan Drutu [email protected]
Fixes #1037
Fixes #1040