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.

Sign up for GitHub

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

Error Flagging with Status Codes #966

Merged
merged 9 commits into from
Sep 24, 2020
Merged

Error Flagging with Status Codes #966

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5cdce91
StatusSource definition
tedsuo Sep 17, 2020
2dff27e
Update HTTP status code mapping
tedsuo Sep 17, 2020
cdb0f04
Remove span status
tedsuo Sep 18, 2020
573c06f
Apply feedback.
carlosalberto Sep 24, 2020
ff6d57c
Remove unused link.
carlosalberto Sep 24, 2020
920dabd
Merge pull request #1 from carlosalberto/error_flagging
tedsuo Sep 24, 2020
61a26f2
Merge remote-tracking branch 'upstream/master'
tedsuo Sep 24, 2020
39a23f0
markdownlint
tedsuo Sep 24, 2020
150be61
spelling
tedsuo Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 26 additions & 49 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,61 +573,38 @@ calling of corresponding API.
## Status

`Status` interface represents the status of a finished `Span`. It's composed of
a canonical code in conjunction with an optional descriptive message.
a canonical code, and an optional descriptive message.

### StatusCanonicalCode

`StatusCanonicalCode` represents the canonical set of status codes of a finished
`Span`, following the [Standard GRPC
codes](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md):
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

- `Unset`
- The default status.
- `Error`
- The operation contains an error.
- `Ok`
- The operation completed successfully.
- `Cancelled`
- The operation was cancelled (typically by the caller).
- `Unknown`
- An unknown error.
- `InvalidArgument`
- Client specified an invalid argument. Note that this differs from
`FailedPrecondition`. `InvalidArgument` indicates arguments that are problematic
regardless of the state of the system.
- `DeadlineExceeded`
- Deadline expired before operation could complete. For operations that change the
state of the system, this error may be returned even if the operation has
completed successfully.
- `NotFound`
- Some requested entity (e.g., file or directory) was not found.
- `AlreadyExists`
- Some entity that we attempted to create (e.g., file or directory) already exists.
- `PermissionDenied`
- The caller does not have permission to execute the specified operation.
`PermissionDenied` must not be used if the caller cannot be identified (use
`Unauthenticated1` instead for those errors).
- `ResourceExhausted`
- Some resource has been exhausted, perhaps a per-user quota, or perhaps the
entire file system is out of space.
- `FailedPrecondition`
- Operation was rejected because the system is not in a state required for the
operation's execution.
- `Aborted`
- The operation was aborted, typically due to a concurrency issue like sequencer
check failures, transaction aborts, etc.
- `OutOfRange`
- Operation was attempted past the valid range. E.g., seeking or reading past end
of file. Unlike `InvalidArgument`, this error indicates a problem that may be
fixed if the system state changes.
- `Unimplemented`
- Operation is not implemented or not supported/enabled in this service.
- `Internal`
- Internal errors. Means some invariants expected by underlying system has been
broken.
- `Unavailable`
- The service is currently unavailable. This is a most likely a transient
condition and may be corrected by retrying with a backoff.
- `DataLoss`
- Unrecoverable data loss or corruption.
- `Unauthenticated`
- The request does not have valid authentication credentials for the operation.
- The operation has been validated by an Application developers or Operator to
have completed successfully, or contain

The status code SHOULD remain unset, except for the following circumstances:

When the status is set to `ERROR` by Instrumentation Libraries, the status codes
SHOULD be documented and predictable. The status code should only be set to `ERROR`
according to the rules defined within the semantic conventions. For operations
not covered by the semantic conventions, Instrumentation Libraries SHOULD
publish their own conventions, including status codes.

Generally, Instrumentation Libraries SHOULD NOT set the status code to `Ok`,
unless explicitly configured to do so. Instrumention libraries SHOULD leave the
status code as `Unset` unless there is an error, as described above.

Application developers and Operators may set the status code to `Ok`.

Analysis tools SHOULD respond to an `Ok` status by supressing any errors they
would otherwise generate. For example, to supress noisy errors such as 404s.

### Status creation

Expand All @@ -650,9 +627,9 @@ Returns the `StatusCanonicalCode` of this `Status`.
Returns the description of this `Status`.
Languages should follow their usual conventions on whether to return `null` or an empty string here if no description was given.

### GetIsOk
### GetIsError
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove all getters from the status API, we don't have getters anywhere else in the API. Although that should be done outside this PR. But I would remove this getter, or keep the name GetIsOK and return true unless set to Error. Otherwise I expect coding mistakes could result from inverting the meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oberon00 @arminru can we also remove GetCanonicalCode and GetDescription? (In a separate PR)

Copy link
Member

Choose a reason for hiding this comment

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

I think we can indeed, in a separate PR.
Thank's for resolving the original issue in this comment ✔️


Returns true if the canonical code of this `Status` is `Ok`, otherwise false.
Returns true if the canonical code of this `Status` is `Error`, otherwise false.

## SpanKind

Expand Down
29 changes: 13 additions & 16 deletions specification/trace/semantic_conventions/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,19 @@ Don't set the span status description if the reason can be inferred from `http.s
| HTTP code | Span status code |
|-------------------------|-----------------------|
| 100...299 | `Ok` |
| 3xx redirect codes | `DeadlineExceeded` in case of loop (see above) [1], otherwise `Ok` |
| 401 Unauthorized ⚠ | `Unauthenticated` ⚠ (Unauthorized actually means unauthenticated according to [RFC 7235][rfc-unauthorized]) |
| 403 Forbidden | `PermissionDenied` |
| 404 Not Found | `NotFound` |
| 429 Too Many Requests | `ResourceExhausted` |
| 499 Client Closed | `Cancelled` (Not an official HTTP status code, defined by [NGINX][nginx-http-499]) |
| Other 4xx code | `InvalidArgument` [1] |
| 501 Not Implemented | `Unimplemented` |
| 503 Service Unavailable | `Unavailable` |
| 504 Gateway Timeout | `DeadlineExceeded` |
| Other 5xx code | `Internal` [1] |
| Any status code the client fails to interpret (e.g., 093 or 573) | `Unknown` |

Note that the items marked with [1] are different from the mapping defined in the [OpenCensus semantic conventions][oc-http-status].

[oc-http-status]: https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#mapping-from-http-status-codes-to-trace-status-codes
| 3xx redirect codes | `Error` in case of loop (see above), otherwise `Ok` |
tedsuo marked this conversation as resolved.
Show resolved Hide resolved
| 401 Unauthorized ⚠ | `Error` ⚠ (Unauthorized actually means unauthenticated according to [RFC 7235][rfc-unauthorized]) |
| 403 Forbidden | `Error` |
| 404 Not Found | `Error` |
| 429 Too Many Requests | `Error` |
| 499 Client Closed | `Error` (Not an official HTTP status code, defined by [NGINX][nginx-http-499]) |
| Other 4xx code | `Error` |
| 501 Not Implemented | `Error` |
| 503 Service Unavailable | `Error` |
| 504 Gateway Timeout | `Error` |
| Other 5xx code | `Error` |
| Any status code the client fails to interpret (e.g., 093 or 573) | `Error` |

[rfc-unauthorized]: https://tools.ietf.org/html/rfc7235#section-3.1
[nginx-http-499]: https://httpstatuses.com/499

Expand Down