opentelemetry-go: Deviation from semconv exception spec?

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 😃

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 1
Comments: 23 (21 by maintainers)

Most upvoted comments

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.

Aneurysm9 on Feb 10, 2021

@Aneurysm9 yes, that is correct 👍

MrAlias on Apr 1, 2021

👋 Hi friends, just a bit confused here on this comment https://github.com/open-telemetry/opentelemetry-go/issues/1491#issuecomment-793127634. So as it stands now is the idea that Go will just not be recording events in a way where type/stacktrace etc will get recognized by anything else in the opentelemetry ecosystem? That feels sub optimal. Exception events are what basically all exporters look for, here’s an example of a popular exporter that only looks at exception events to determine cause and add the additional useful metadata. I don’t believe there’s a similar error event convention? Feels like we’re sacrificing quality of adoption today for future hypothetical work. https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/e2af59e6cbd5f3e384a5e85d980c7a1fab22b663/exporter/awsxrayexporter/translator/cause.go#L43-L81

ericmustin on Mar 11, 2021

@MrAlias Happy to help! My bandwidth is a little tight this week, but I’ll be able to get to it either over the weekend or early next week. If I’m lucky I might get to it sooner 😃

mothershipper on Feb 17, 2021

@bhautikpip the plan is to go with the strategy describe by @Aneurysm9 above: https://github.com/open-telemetry/opentelemetry-go/issues/1491#issuecomment-777082014 with the addition of ensuring the panic related event that is created is created with the proper name.

@mothershipper I have assigned this to you because you had an open issue to address this. Please let me know if you are still able to work on this and update the linked PR with this decided direction.

MrAlias on Feb 17, 2021

@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

XSAM on Jan 27, 2021