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

Exception semantic conventions and Go error return values #764

Open
Oberon00 opened this issue Aug 7, 2020 · 12 comments
Open

Exception semantic conventions and Go error return values #764

Oberon00 opened this issue Aug 7, 2020 · 12 comments
Labels
area:error-reporting Related to error reporting area:semantic-conventions Related to semantic conventions release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@Oberon00
Copy link
Member

Oberon00 commented Aug 7, 2020

See #761 (comment).

For Go errors, see https://blog.golang.org/go1.13-errors. Go errors are not exceptions, as the defining characteristic is that it has some kind of special throw/try/catch control flow that facilitates separating the "happy path" from the "error path".

Currently we seem to implicitly encourage this in https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md#stacktrace-representation that Go should use runtime.debug.Stack (a function that returns the current stack, not directly associated with error handling or error objects).

On the other hand, go also has panics https://blog.golang.org/defer-panic-and-recover which seem to match exceptions far better.

Rust may require similar treatment (it also has panics and a Result type), and some C++ applications (using Boost.Outcome, std::expected, etc).

I can think of two solutions:

  1. Make clear that Go panics are fine to use with the exception conventions but Go errors are not.
  2. Describe in more detail how Go errors are mapped to exceptions.
@Oberon00 Oberon00 added the spec:trace Related to the specification/trace directory label Aug 7, 2020
@Oberon00 Oberon00 added area:error-reporting Related to error reporting area:semantic-conventions Related to semantic conventions labels Aug 7, 2020
@andrewhsu andrewhsu added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p2 Medium priority level labels Aug 7, 2020
@Oberon00
Copy link
Member Author

CC @open-telemetry/go-approvers

@Aneurysm9
Copy link
Member

I think that it makes sense to treat panic/recover like throw/catch. Errors are definitely not exceptions, though sometimes they are used similarly, even if only for the fact that they can be easily (even inadvertently) ignored, which stops propagation through the call stack.

@tigrannajaryan
Copy link
Member

This is a blocker for Go, but is this a blocker for the spec? Do we believe that while working on this we may find that Go requires something that makes current exception specification impossible to use?

@Oberon00
Copy link
Member Author

Oberon00 commented Sep 9, 2020

I don't think so. We might just discover that Go needs additional attributes or a different convention. E.g. maybe Go will use an event attribute "unwinding=false" to distinguish panics from error returns. So I think we can remove the required-for-ga here.

@Oberon00 Oberon00 added release:after-ga Not required before GA release, and not going to work on before GA and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 9, 2020
@andrewhsu
Copy link
Member

@open-telemetry/go-maintainers ptal, fyi this one was moved to after-ga, and respond if this is not desirable.

@andrewhsu andrewhsu removed the priority:p2 Medium priority level label Sep 9, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Sep 9, 2020

Looking at the specification for handling exceptions I wonder if the canonical Go implementation would fit. Currently we provide a way to record errors with a specific method of a span. This seems in line with the purpose of the specification on exception handling. However, the canonical way to deal with panics in Go is to recover in a deferred function that is run when the current scope ends. This means that we have added panic handling in the Span.End method to add an appropriate event capturing the relevant information.

I think this is in line with the spirit of what the specification is trying to solve, but it does not fit the specific language of the specification. If this seems fine and we all agree that it would not block the Go implementation from saying it implements the spec I'm fine with waiting post-GA to update the language to accommodate this implementation.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 23, 2021

It looks like this may indeed block the OTel Go release based on the fact that backends are using the exception semantics to represent all errors within the system and Go does not send errors in this form.

@Oberon00
Copy link
Member Author

Oberon00 commented Mar 24, 2021

I don't think languages are supposed to invent & use their own semantic conventions without submitting them as a PR to the spec. So if go uses an error. prefix but otherwise the same semantic conventions, I think we need to do something, not (only) in backends. We could try to put that into the spec as used by Go now. But I wonder what e.g. Rust uses (do we have any other OTel languages that also use errors instead of exceptions?).

If exception conventions are a good fit except for the name, I would actually prefer adding a new attribute to the convention that describes the kind of "exception", i.e. whether it is a classical stack-unwinding exception or something else.

See also https://cloud-native.slack.com/archives/C01N7PP1THC/p1615194505073700?thread_ts=1614978903.067900&cid=C01N7PP1THC and #764 (comment)

@Aneurysm9
Copy link
Member

I believe that Rust has decided to emit exception., even though they use errors. We have a PR in the Go repo that would change our RecordError method to also produce events with the exception. attributes. I'm fine with landing that, despite the conceptual mismatch, precisely because consistency for downstream processors is important. If we could add a new attribute to communicate the category or form of the exception/error that may be a good way to bridge some of that conceptual gap.

@Oberon00
Copy link
Member Author

I think this issue needs to picked up again, if we want to get semantic conventions stable.

@jsuereth
Copy link
Contributor

I'm going to re-escalate this. It's not just a Go issue. As called out, Rust "result" type has this concern, as would any "wrapper"-style error-handling language (Scala, Haskell, etc.)

Related #3495

@tsloughter
Copy link
Member

Seems also related to #3198

Erlang also uses wrapper style error handling (along with exceptions) and I'd support the move to error to replace exception, along with stacktrace being optional, so errors can be properly reported without conflating with exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:error-reporting Related to error reporting area:semantic-conventions Related to semantic conventions release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

7 participants