Deviation from semconv exception spec?

[mothershipper]

Just noticed that the trace implementation doesn't seem to follow the spec for exceptions -- looks like this package uses error as the prefix instead of exception. Looking at the JS library, they seem to be using exception as well, so I'm guessing it's the golang lib that's out of compliance.

Are there any plans or motivation to bring this into compliance? I don't know if there's a backwards compatible way of making this change, aside of perhaps recording two events, one with the error prefix, one with the exception prefix. That said, it might be best to just rip the bandaid off, rename the event and attributes and make it a (potentially) breaking change?

Happy to contribute these changes if I can get some direction on what you think is the best path forward :)

[XSAM]

@mothershipper Good catch.

From Record Exception

The signature of the method is to be determined by each language
and can be overloaded as appropriate.

I'm not sure we can keep RecordError by appropriately changing the signature of Record Exception. After all, there is no term called Exception in Go; RecordException might confuse the users from Go.

I prefer we should correct semantic conventions but keep the RecordError.

@MrAlias @Aneurysm9 What do you think?

Related issue: #1104

[mothershipper]

I'm on the fence about the method name, but I'm doubtful that Exception would confuse go developers -- if this were the case, I'd think adding a span event as an exception from a RecordError method would be equally as confusing.

My gut feeling is that the terms are relatively interchangeable to developers. For instance, JavaScript doesn't have an Exception as best I can tell either. They use an Error class, but the SDK provides a recordException method. That said, the JS library uses Exception in the method signature, but their Exception type is satisfied with a JS Error.

To me, the bigger concern with changing the name is that it is part of the public API and renaming would be a breaking change. Would you consider adding RecordException and deprecating RecordError over removal?

[Aneurysm9]

I don't think a change in method name is required. RecordException is optional in the spec and scoped to languages that use exceptions, which Go does not.

To facilitate recording an exception languages SHOULD provide a RecordException method if the language uses exceptions.

Instead, we have errors and provide an analogous RecordError method. Obviously, this wasn't intended to preclude us from having a convenience method, but I think it also makes clear that where exceptions are not part of a language different behavior and API surface is appropriate.

It may be a weird aesthetic thing, and I'm happy to be told I'm wrong if there's overwhelming thoughts to the contrary, but I don't feel that renaming RecordError to RecordException is desirable or beneficial. I would find it odd to encounter span.RecordException(err) throughout a codebase and I'd prefer not to do anything to encourage the conflating of errors and exceptions.

[mothershipper]

@Aneurysm9 @XSAM Thank you both for the reviews!

I would find it odd to encounter span.RecordException(err) throughout a codebase and I'd prefer not to do anything to encourage the conflating of errors and exceptions.

Looking for clarity here -- would you have RecordError add an exception event with exception.* attributes? Or leave it as-is?

[Aneurysm9]

I think I would prefer to leave it as-is, though I can see the argument for using exception.* to align downstream with attributes produced in a heterogeneous environment.

[mothershipper]

For reference the majority of opentelemetry SDKs have a RecordException with exception.* semantics, though, most of these languages do have throw/catch semantics:

• C#
• Python
• Java
• JS
• Ruby
• Rust

The only ones without:

• C++ - Not implemented either way
• PHP - Not implemented either way
• go - see above ;)

I think the strongest point I can make is that the Rust SDK supports record_exception, despite not having exceptions. This issue was brought up (I'm assuming before the Rust SDK addressed this) in the spec as well.

From a trace-visualization perspective, our end-users just want to know why an operation failed, they don't much care if it was an exception or an error. Our users want to see the type, message and source/stack-trace (if applicable) when a failure escapes a span. I don't think the spec currently provides a standard way of doing that outside of the exception event, and it's definitely easier on the various exporter(s) if golang were to conform with the other SDKs, at least by using exception.* semantics, regardless of what the method is called.

[Aneurysm9]

I've added an agenda item for the SIG meeting tomorrow to discuss this as I'd like to get input from a broader set of users and approvers. My personal preference at this point is to retain the RecordError() function but have it produce exception.* attributes on an exception event. This is partly selfish as I'm leaving a team with a reasonably large body of OTel-instrumented code with broad use of RecordError(), but also because I believe it aligns better with the Go idiom.

[mothershipper]

Thanks Anthony!

For what it's worth, I don't feel as strongly about the method name as I do about the behavior (producing the exception event). There is value in consistency, but also in avoiding breaking changes / not inflicting maintenance work on the community.

[MrAlias]

From the SIG meeting today: we will also want to update the Event name that is created when a panic is recovered from (an re-paniced):

opentelemetry-go/sdk/trace/span.go

Line 37 in e50a1c8

errorEventName = "error"