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.

Sign up for GitHub

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

Jump to bottom

Exception reporting specification #697

Merged
merged 14 commits into from
Jul 17, 2020
Merged

Exception reporting specification #697

merged 14 commits into from
Jul 17, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Jul 9, 2020

This PR is another take on exception / error reporting. It's based on #427 and conversations from the errors working group. Below is a summary of differences between this proposal and #427:

  • Use exception terminology rather than error. Whether or not an exception is an error will depend on context.
  • Change stacktrace from array of strings, to a single string
    • The format of the string is to be determined on a language by language basis
    • Structure can be derived from the string format and platform information, if necessary
  • Add a Span.RecordException API method

The goal of this PR is to solve the 90% use case, without painting ourselves into a corner for future improvements.

@mwear mwear requested review from a team July 9, 2020 23:50
@mwear mwear force-pushed the exception_semantic_conventions branch from 268225d to 4a1fbe8 Compare July 9, 2020 23:51
@mwear mwear mentioned this pull request Jul 10, 2020
Closed
Aneurysm9
Aneurysm9 approved these changes Jul 10, 2020
View reviewed changes
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.

This aligns very well with what the Go SDK has implemented, though we have RecordError() rather than RecordException(). That's a pretty easy change for us to make at this point. I like it.

@arminru arminru mentioned this pull request Jul 10, 2020
Closed
@kbrockhoff
Copy link
Member

This looks like just what we need for the 1.0 GA release. It is very important to get this merged so work needed for the SDK GA releases can proceed.

@Oberon00
Copy link
Member

Oberon00 commented Jul 10, 2020 •
edited
Loading

LGTM but I strongly recommend to decide on open-telemetry/oteps#123 before merging this (even if the decision is just "we will use #697 for GA and consider open-telemetry/oteps#123 later).

Oberon00
Oberon00 reviewed Jul 10, 2020
View reviewed changes
specification/trace/exception-reporting.md Outdated

## API

To facilitate recording an exception languages SHOULD provide a
Copy link
Member

Choose a reason for hiding this comment

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

For an alternative idea see open-telemetry/oteps#123 (comment) and following comment.

flands
flands reviewed Jul 10, 2020
View reviewed changes
Copy link

@flands flands left a comment

Choose a reason for hiding this comment

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

Just catching up on this working group now -- great work everyone!

General comment: this is very exception focused, but an exception is just a type of error. OpenTracing semantic conventions based everything on errors (https://opentracing.io/specification/conventions/) so you could have an error.kind of exception and then all key names would be based on error (e.g. error.type instead of exception.type). Was this approach considered?

specification/trace/exception-reporting.md Outdated Show resolved Hide resolved
specification/trace/exception-reporting.md Outdated Show resolved Hide resolved
specification/trace/exception-reporting.md Outdated Show resolved Hide resolved
specification/trace/exception-reporting.md Outdated Show resolved Hide resolved
specification/trace/exception-reporting.md Outdated Show resolved Hide resolved
@iNikem
Copy link
Contributor

iNikem commented Jul 10, 2020

Just catching up on this working group now -- great work everyone!

General comment: this is very exception focused, but an exception is just a type of error. OpenTracing semantic conventions based everything on errors (https://opentracing.io/specification/conventions/) so you could have an error.kind of exception and then all key names would be based on error (e.g. error.type instead of exception.type). Was this approach considered?

@flands The general sentiment was exactly the opposite: we record an exception, which may or may not be an error.

@carlosalberto carlosalberto added area:error-reporting Related to error reporting spec:trace Related to the specification/trace directory area:api Cross language API specification issue release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jul 10, 2020
@Oberon00 Oberon00 added the area:semantic-conventions Related to semantic conventions label Jul 10, 2020
@iNikem
Copy link
Contributor

iNikem commented Jul 14, 2020

Thanks for removing the severity.
The only other point I feel rather strongly about is that I'd like to have the stacktrace attribute renamed to something with exception. in front. See #697 (comment). EDIT: Alternatively, I would also be fine with removing stacktrace entirely from this PR.

Please don't remove! :) Both name variants are good to me.

Oberon00
Oberon00 approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! 👍

EDIT: I think it is completely fine to move the discussion in #697 (comment) to a follow up.

The bikeshedding in #697 (comment) about exception.stacktrace vs something that does not make one believe it is OK to only put the stacktrace without exception message there would be nice to also resolve before merging because it is a breaking change, but I can create a follow-up PR for this too.

@iNikem
Copy link
Contributor

iNikem commented Jul 14, 2020

The bikeshedding in #697 (comment) about exception.stacktrace vs something that does not make one believe it is OK to only put the stacktrace without exception message there would be nice to also resolve before merging because it is a breaking change, but I can create a follow-up PR for this too.

I stay on my opinion that this should be left to language SIG. To specify a recommended format of that field content.

@iNikem iNikem mentioned this pull request Jul 16, 2020
Closed
This was referenced Jul 16, 2020
Closed
Closed
@iNikem
Copy link
Contributor

iNikem commented Jul 17, 2020

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers can somebody merge this, please?

@yurishkuro yurishkuro merged commit 29fccfe into open-telemetry:master Jul 17, 2020
@jtescher jtescher mentioned this pull request Jul 17, 2020
Closed
codeboten pushed a commit to codeboten/opentelemetry-specification that referenced this pull request Jul 20, 2020
@mwear @arminru
Exception reporting specification (open-telemetry#697)
73db666 
* Add semantic conventions for errors

* Update error.type description

Co-Authored-By: Armin Ruech <[email protected]>

* update semantic conventions for exceptions

* add details for attributes; remove log.severity; add API details

* rename exceptions.md -> exception-reporting.md

* more accurately describe format for java

* alphabetize languages in stacktrace representation table

* prefer dynamic type over static type

* add log.severity convention

* Revert "add log.severity convention"

This reverts commit 137556e.

* rename stacktrace -> exception.stacktrace

* move file to semantic conventions directory

* move RecordException to API

* fix formatting

Co-authored-by: Armin Ruech <[email protected]>
@iNikem iNikem mentioned this pull request Jul 22, 2020
Closed
@Oberon00 Oberon00 mentioned this pull request Jul 28, 2020
Closed
@iNikem iNikem mentioned this pull request Jul 30, 2020
Closed
This was referenced Aug 6, 2020
Closed
Merged
Merged
This was referenced Aug 12, 2020
Closed
Closed
@mwear mwear mentioned this pull request Aug 18, 2020
Closed
@Oberon00 Oberon00 mentioned this pull request Aug 22, 2020
Merged
@Oberon00 Oberon00 mentioned this pull request Dec 30, 2020
Closed
@MrAlias MrAlias mentioned this pull request Mar 23, 2021
Closed
@mwear mwear mentioned this pull request Mar 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@flands flands flands left review comments

@Oberon00 Oberon00 Oberon00 approved these changes

@jkwatson jkwatson jkwatson left review comments

@iNikem iNikem iNikem approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan left review comments

@johnbley johnbley johnbley left review comments

@arminru arminru arminru left review comments

@tedsuo tedsuo tedsuo approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@jmacd jmacd jmacd approved these changes

Assignees
No one assigned
Labels
area:api Cross language API specification issue area:error-reporting Related to error reporting area:semantic-conventions Related to semantic conventions release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.