Skip to content

Adds semantic conventions for exceptions#1492

Merged
MrAlias merged 1 commit intoopen-telemetry:mainfrom
mothership:task/jack/semconv-exceptions
Apr 1, 2021
Merged

Adds semantic conventions for exceptions#1492
MrAlias merged 1 commit intoopen-telemetry:mainfrom
mothership:task/jack/semconv-exceptions

Conversation

@mothershipper
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread semconv/exception.go Outdated
Comment thread semconv/exception.go Outdated
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

👍 rebased and pushed

@MrAlias MrAlias mentioned this pull request Apr 23, 2021
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