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

Jump to bottom

Do not set error status when recording an error with RecordError #1661

Closed
MrAlias opened this issue Mar 5, 2021 · 4 comments · Fixed by #1663 or #1729
Closed

Do not set error status when recording an error with RecordError #1661

MrAlias opened this issue Mar 5, 2021 · 4 comments · Fixed by #1663 or #1729
Labels
area:trace Part of OpenTelemetry tracing good first issue Good for newcomers pkg:SDK Related to an SDK package
Milestone
RC1

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Mar 5, 2021

From the specification:

The status code should only be set to ERROR according to the rules defined within the semantic conventions.

However we set the span status when recording an error:

opentelemetry-go/sdk/trace/span.go

Line 265 in 37688ef

s.SetStatus(codes.Error, "")

It was pointed out in this thread that there could be a situation where an error is encountered but handled so when it is recorded the user may not want the span to be considered errored.

We should remove this line to ensure we are in compliance with the specification. Additionally, we need to be clear of this behavior in the method documentation (that it will not set the span status).
@MrAlias MrAlias added good first issue Good for newcomers pkg:SDK Related to an SDK package area:trace Part of OpenTelemetry tracing release:required-for-ga labels Mar 5, 2021
@MrAlias MrAlias added this to the RC1 milestone Mar 5, 2021
This was referenced Mar 5, 2021
Closed
Closed
@mrveera
Copy link
Contributor

mrveera commented Mar 5, 2021

@MrAlias Picking this up

mrveera added a commit to mrveera/opentelemetry-go that referenced this issue Mar 5, 2021
@mrveera
Fix open-telemetry#1661 Removed setting error status while recording …
5b3fffd 
…err as span event
@mrveera mrveera mentioned this issue Mar 5, 2021
Merged
mrveera added a commit to mrveera/opentelemetry-go that referenced this issue Mar 5, 2021
@mrveera
Fix open-telemetry#1661 Removed setting error status while recording …
3ec3208 
…err as span event
@seh
Copy link
Contributor

seh commented Mar 5, 2021

Should we introduce a functional option for span.RecordError to request that it set the status as well if the supplied error is not nil?

@mrveera
Copy link
Contributor

mrveera commented Mar 6, 2021

@seh Sounds like good idea that users don't need to set it separately when needed. they can invoke this function with respective argument

MrAlias added a commit that referenced this issue Mar 8, 2021
@mrveera @Aneurysm9 @MrAlias
Removed setting error status while recording err as span event (#1663)
a5edd79 
* Fix #1661 Removed setting error status while recording err as span event

* Update CHANGELOG.md

Co-authored-by: Anthony Mirabella <[email protected]>

* Update RecordError method doc

Co-authored-by: Tyler Yahn <[email protected]>

* Fix grammatical errors in Changelog

Co-authored-by: Tyler Yahn <[email protected]>

* Update RecordError method doc

Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
@MrAlias MrAlias mentioned this issue Mar 8, 2021
Open
@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 8, 2021

Should we introduce a functional option for span.RecordError to request that it set the status as well if the supplied error is not nil?

@seh tracking this in #1677

@XSAM XSAM mentioned this issue Mar 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:trace Part of OpenTelemetry tracing good first issue Good for newcomers pkg:SDK Related to an SDK package
Projects
None yet
Milestone
RC1
3 participants
@seh @MrAlias @mrveera