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

Change Status to be consistent with Link and Event #1067

Merged
merged 7 commits into from
Oct 7, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ release.

New:

- Change Status to be consistent with Link and Event
([#1067](https:/open-telemetry/opentelemetry-specification/pull/1067))
- Clarify env variables in otlp exporter
([#975](https:/open-telemetry/opentelemetry-specification/pull/975))
- Add Prometheus exporter environment variables
Expand Down
109 changes: 42 additions & 67 deletions specification/trace/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@ Table of Contents
* [Record Exception](#record-exception)
* [Span lifetime](#span-lifetime)
* [Propagated Span creation](#propagated-span-creation)
* [Status](#status)
* [StatusCanonicalCode](#statuscanonicalcode)
* [Status creation](#status-creation)
* [GetCanonicalCode](#getcanonicalcode)
* [GetDescription](#getdescription)
* [SpanKind](#spankind)
* [Concurrency](#concurrency)
* [Included Propagators](#included-propagators)
Expand Down Expand Up @@ -496,16 +491,52 @@ Note that [`RecordException`](#record-exception) is a specialized variant of

#### Set Status

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`.
Copy link
Member Author

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)


Only the value of the last call will be recorded, and implementations are free
to ignore previous calls.
`Status` is semantically defined by the following properties:

- `StatusCanonicalCode` represents the canonical set of `Status` codes.
Copy link
Member

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?

Copy link
Member Author

@bogdandrutu bogdandrutu Oct 6, 2020

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.

- Description represents the descriptive message of the `Status`.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

`StatusCanonicalCode` has the following values:

- `Unset`
- The default status.
- `Ok`
- The operation has been validated by an Application developers or Operator to
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
have completed successfully, or contain
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
- `Error`
- The operation contains an error.

The Span interface MUST provide:

- An API to set the `Status` where the new status is the only argument. This
SHOULD be called `SetStatus`.
- An API to set the `Status` where the new `Status` properties are passed as
arguments. This SHOULD be called `SetStatus`. This API takes the
`StatusCanonicalCode`, and optional description.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

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 suppressing any errors they
would otherwise generate. For example, to suppress noisy errors such as 404s.
Copy link
Member

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?

Copy link
Member Author

@bogdandrutu bogdandrutu Oct 6, 2020

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.


Only the value of the last call will be recorded, and implementations are free
to ignore previous calls.
Comment on lines +536 to +537
Copy link
Member

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.


Note that there are multiple ways to implement this API, such as: overloading,
variadic arguments, or define an immutable object to be passed to the API calls.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

#### UpdateName

Expand Down Expand Up @@ -594,62 +625,6 @@ The remaining functionality of `Span` MUST be defined as no-op operations.

This functionality MUST be fully implemented in the API, and SHOULD NOT be overridable.

## Status

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

### StatusCanonicalCode

`StatusCanonicalCode` represents the canonical set of status codes of a finished
`Span`.

- `Unset`
- The default status.
- `Error`
- The operation contains an error.
- `Ok`
- 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 suppressing any errors they
would otherwise generate. For example, to suppress noisy errors such as 404s.

### Status creation

API MUST provide a way to create a new `Status`.

Required parameters

- `StatusCanonicalCode` of this `Status`.

Optional parameters

- Description of this `Status`.

### GetCanonicalCode

Returns the `StatusCanonicalCode` of this `Status`.

### GetDescription

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.

## SpanKind

`SpanKind` describes the relationship between the Span, its parents,
Expand Down
2 changes: 1 addition & 1 deletion specification/trace/semantic_conventions/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ default span name.

## Status

[Span Status](../api.md#status) MUST be left unset if HTTP status code was in the
[Span Status](../api.md#set-status) MUST be left unset if HTTP status code was in the
1xx, 2xx or 3xx ranges, unless there was another error (e.g., network error receiving
the response body; or 3xx codes with max redirects exceeded), in which case status
MUST be set to `Error`.
Expand Down