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

Remove Span.Status per OTEP-134 #918

Closed
wants to merge 6 commits into from

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Sep 3, 2020

Fixes #706

Changes

Remove Span.Status as per OTEP-134

@@ -371,8 +363,8 @@ Links SHOULD preserve the order in which they're set.

### Span operations

With the exception of the function to retrieve the `Span`'s `SpanContext` and
recording status, none of the below may be called after the `Span` is finished.
Copy link
Member

@Oberon00 Oberon00 Sep 3, 2020

Choose a reason for hiding this comment

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

Note: This is wrong even in the current spec before removing status.

specification/trace/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
iNikem and others added 2 commits September 3, 2020 13:32
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -35,7 +35,6 @@ status of the feature is not known.
|End | + | + | + | + | + | + | + | + | | + |
|End with timestamp | + | + | + | + | + | + | + | - | | + |
|IsRecording | + | + | + | + | + | + | + | | | + |
|Set status | + | + | + | + | + | + | + | + | | + |
Copy link
Member

Choose a reason for hiding this comment

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

We could keep this and add something like (REMOVED) to keep track of its removal process (until all SIGs are done).

specification/trace/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
iNikem and others added 3 commits September 3, 2020 15:34
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Approving in the sense of "I (and Dynatrace) would be OK with this solution" (even without replacement, as http.status_code et al is enough for us).

Replacing status with unspecified/ok/error would also be fine with me (we would probably still not use it at Dynatrace though as we prefer more specific error information).

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 12, 2020
@tedsuo
Copy link
Contributor

tedsuo commented Sep 17, 2020

@iNikem do you mind closing this PR, now that we have OTEP 136? Related issue: #965.

@iNikem
Copy link
Contributor Author

iNikem commented Sep 17, 2020

Closed in favor of open-telemetry/oteps#136

@iNikem iNikem closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Span.Status from API
6 participants