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

Adds semantic conventions for exceptions #1492

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Adds semantic conventions for exceptions #1492

merged 1 commit into from
Apr 1, 2021

Conversation

mothershipper
Copy link
Contributor

@mothershipper mothershipper commented Jan 26, 2021

Alters RecordError to support the opentelemetry semantic conventions for exceptions. In
short, this renames the error event to exception and updates the attribute keys.*

Fixes #1491

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #1492 (c9249f6) into main (928e3c3) will not change coverage.
The diff coverage is 75.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1492   +/-   ##
=====================================
  Coverage   78.5%   78.5%           
=====================================
  Files        133     133           
  Lines       7083    7083           
=====================================
  Hits        5567    5567           
  Misses      1270    1270           
  Partials     246     246           
Impacted Files Coverage Δ
bridge/opentracing/internal/mock.go 73.6% <0.0%> (ø)
trace/trace.go 86.4% <ø> (ø)
oteltest/span.go 100.0% <100.0%> (ø)
sdk/trace/span.go 94.0% <100.0%> (ø)

Base automatically changed from master to main January 29, 2021 19:39
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

I would like to separate the change to semantic conventions from the decision whether to use RecordError, RecordException, or both.

@mothershipper mothershipper changed the title Adds semantic conventions for exceptions [WIP] Adds semantic conventions for exceptions Feb 17, 2021
@mothershipper mothershipper changed the title [WIP] Adds semantic conventions for exceptions Adds semantic conventions for exceptions Feb 20, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
semconv/exception.go Outdated Show resolved Hide resolved
semconv/exception.go Outdated Show resolved Hide resolved
@MrAlias MrAlias requested a review from Aneurysm9 March 3, 2021 16:37
@MrAlias MrAlias added this to the RC1 milestone Mar 4, 2021
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Blocking this until a resolution can be decided on in #1491

@MrAlias
Copy link
Contributor

MrAlias commented Apr 1, 2021

I'm not able to push to this branch. @mothershipper can you resolve the conflicts so we can merge this?

Adds support for the opentelemetry exceptions semantic conventions. In
short, this has RecordError produce an exception event with exception
attributes instead of using the error event and error attributes.

While golang does not have exceptions, the spec itself does not
differentiate between errors and exceptions for recording purposes.
RecordError was kept as the method name, both for backwards
compatibility and to reduce confusion (the method signature takes in a
golang error object). The spec appears to allow this, as it suggests the
method is optional and signature may reflect whatever is most appropriate
for the language implementing it.

It may seem non-intuitive to log an exception event from a method called
RecordError, but it's beneficial to have consistent behavior across all
opentelemetry SDKs. Downstream projects like the opentelemetry-collector
can build off of the published API and not special case behaviors from
individual languages.
@mothershipper
Copy link
Contributor Author

👍 rebased and pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deviation from semconv exception spec?
4 participants