Add exception.escaped attribute. #784

Oberon00 · 2020-08-11T16:49:56Z

Originally discussed in #697 (comment).

This was split from ~~and requires~~ #761. See also discussion there.

~~Depends on #814 ✔️ for the RecordException change.~~ The API addition was removed on @bogdandrutu's request, see #784 (comment) (and earlier #784 (comment))

Changes

Add exception.escaped attribute to mark definitely escaped exceptions.

This reverts commit 80ea27d.

arminru

Once #761 is merged you should also update the code sample there to properly set escaped=true.

bogdandrutu · 2020-08-12T20:22:38Z

specification/trace/api.md

+- `RecordException(exception: Exception, escaped: boolean? = null)`
+- `RecordException(type: String, message: String, stacktrace: String, escaped: boolean?)`


I would like to think this way:
"Is this information so important that deserves being a first class concept in our API"?

Also are there any other fields that are optional? If we add the optional concept to this API does it apply to other fields like stacktrace?

"Is this information so important that deserves being a first class concept in our API"?

I think so (given that we have already decided that exceptions are important enough). More importantly, there is no way to customize the event created by recordException, so you have to add API support for it to make it practically usable at all.

Also are there any other fields that are optional? If we add the optional concept to this API does it apply to other fields like stacktrace?

Interesting thought, especially since retrieving (and exporting) the stacktrace might be expensive. I created #814 to track this.

If we add the optional concept to this API

Since this is marked after-ga, I had to make it optional to avoid breakage. But now it seems this PR already has enough approvals to get it merged for GA. I would be fine with making these required parameters (maybe in a follow-up to not invalidate approvals) thus avoiding the "optional concept". WDYT?

Or way your question less about the default = null but more about the ? type that allows null? I think allowing to not set the attribute is required here since you don't know in all situations.

I personally prefer it as an optional parameter (it would also mean merging this PR sooner, as a further conversation of making it required would happen in a follow up).

@bogdandrutu any objection to PR this merge? Although it indeed has enough approvals, you raised an issue here ;)

Still not convinced:

If we go with attributes associated with the "RecordException" then this is not needed, and causes us pain if released for GA and we need to remove;

I would prefer to focus on the current problem with the API which is that all arguments are required because adding new arguments like this should be backwards compatible but removing others is backwards incompatible;

Based on this I would not rush especially now before GA to add new "features" that are not proven - I mean we should have some prototype, etc.

and causes us pain if released for GA and we need to remove;

I don't think we will ever have to remove this. If anything this becomes an overload of the function that does a trivial translation to some other overload that accepts an arbitrary number of attributes.
However, we also have the same topic of adding a parameter to RecordException on #822 (although that one looks like it is really not necessary IMHO), so we should discuss this a bit more on that PR, because if RecordException becomes a point where everyone wants to add new attributes, this is bad.

We could resolve #814 by replacing recordException with an optional parameter "exception" to addEvent. WDYT? Then this PR would not need to change either recordException or addEvent.

I would prefer to focus on the current problem with the API

That focus is in #814.

That was my point that it looks like there are more ideas about adding new params which, and in some design proposed like using attributes this will become obsolete. Just want to make sure we are not going to have to deprecate this API in 3 months, that is my main concern.

looks like there are more ideas about adding new params

Agreed, we need to decide on something there before merging (this part of) the PR. EDIT: I don't think this can be sensibly split from this PR, because having just the attribute there without a way to set it with RecordException will make people ask how the RecordException implementation would come up with that value.

I think this is resolved now. @bogdandrutu

bogdandrutu

As per comments, blocking this because the focus should be on #814

bogdandrutu · 2020-08-30T15:38:50Z

Should this be updated or dropped now that we have the generic attributes?

Oberon00 · 2020-08-30T19:50:33Z

No, error.hint does not support the same use cases: An escaped exception may still not be an error and vice versa. I will rebase the PR after the weekend 😉

bogdandrutu · 2020-08-31T00:33:54Z

I was referring to generic attributes not to error hint (see https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#record-exception) so this may be an attribute not a special argument passed to the function

Oberon00 · 2020-09-01T09:19:23Z

@bogdandrutu (#784 (review)):

As per comments, blocking this because the focus should be on #814

You still have a "request changes" on this PR, but #814 is resolved now. Are you still against merging this PR? Otherwise consider dismissing your "request changes" (I understand you are not convinced enough to approve it).

bogdandrutu · 2020-09-02T14:13:09Z

I am worried that this API will become extremely different between languages which may be ok as long as semantically they are equivalent, but I do believe that so far we did not allow a choice between having something as a semantic convention vs an API concept, and this being the first time doing this worries me, because we will open a lot of cases like this.

So I would prefer to not open another possibility in the API and decide that everything that is a semantic convention is everywhere, and everything that is an API concept is everywhere. I am worried about consistency between languages.

bogdandrutu · 2020-09-02T14:19:18Z

So, indeed this feels like a small change, but as I pointed in the previous comment adds a new "way to implement" specs, so I am worried if this is the right PR to do that. If we want to give that opportunity to the languages we should have it clearly described, and we should discuss it. This is how I see this and that's why I still have required changes

Oberon00 · 2020-09-02T14:49:45Z

@bogdandrutu I removed the API change (under protest!). But please also request changes on the error.hint PR #822 then for the same reasons. I don't want error.hint to become misused as exception.escaped just because it is more convenient to put on the event.

Oberon00 · 2020-09-02T15:26:45Z

Please ping me or @arminru to get this branch updated to merge it (see #512)

bogdandrutu · 2020-09-02T15:28:29Z

First thanks for pointing to the other PR, commented there as well. Secondly please update the branch :)

bogdandrutu · 2020-09-02T20:56:20Z

I remembered that I have to give people a chance to comment 24h at least since the last change was significant. I will merge tomorrow morning

specification/trace/semantic_conventions/exceptions.md

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

Oberon00 · 2020-09-03T18:02:25Z

@bogdandrutu There were a few trivial changes (changelog entry added, typos fixed), but I think this is (still) ready to merge.

Add exception.escaped attribute.

367a6a6

This reverts commit 80ea27d.

Oberon00 requested a review from a team as a code owner August 11, 2020 16:49

Oberon00 requested a review from a team August 11, 2020 16:49

Oberon00 requested a review from a team as a code owner August 11, 2020 16:49

Oberon00 requested a review from bogdandrutu August 11, 2020 16:49

Oberon00 mentioned this pull request Aug 11, 2020

Specify which exceptions SHOULD be recorded #761

Closed

jmacd approved these changes Aug 11, 2020

View reviewed changes

arminru approved these changes Aug 12, 2020

View reviewed changes

arminru reviewed Aug 12, 2020

View reviewed changes

carlosalberto approved these changes Aug 12, 2020

View reviewed changes

Oberon00 requested review from mwear and iNikem August 12, 2020 13:45

bogdandrutu self-assigned this Aug 12, 2020

bogdandrutu reviewed Aug 12, 2020

View reviewed changes

bogdandrutu closed this Aug 13, 2020

bogdandrutu reopened this Aug 13, 2020

This was referenced Aug 17, 2020

Should Span.RecordException() support more options or be replaced by an optional AddEvent() parameter? #814

Closed

Add error.hint semantic convention #822

Closed

bogdandrutu requested changes Aug 18, 2020

View reviewed changes

Oberon00 mentioned this pull request Aug 25, 2020

RecordException: Allow additional attributes. #874

Merged

This comment has been minimized.

Sign in to view

github-actions bot added the Stale label Aug 30, 2020

Remove exception.escaped from the API.

156b8f5

bogdandrutu approved these changes Sep 2, 2020

View reviewed changes

Oberon00 added 2 commits September 2, 2020 17:31

Merge branch 'master' into exception-escaped

09a00b3

Merge branch 'master' into exception-escaped

adc49b4

Oberon00 added 5 commits September 3, 2020 12:12

Add CHANGELOG entry.

e74896f

Wrong PR# in CHANGELOG.

bd409ce

Wrong wrong PR# in CHANGELOG (hey anagrams!).

9fad2e8

Closing )

207ea30

Merge branch 'master' into exception-escaped

2b02c8c

arminru reviewed Sep 3, 2020

View reviewed changes

specification/trace/semantic_conventions/exceptions.md Outdated Show resolved Hide resolved

Typos

55752c0

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

arminru approved these changes Sep 3, 2020

View reviewed changes

Oberon00 added 4 commits September 3, 2020 20:45

Merge branch 'master' into exception-escaped

5d32ade

Merge branch 'master' into exception-escaped

dc61499

Merge branch 'master' into exception-escaped

6ed1c97

Merge branch 'master' into exception-escaped

5121411

bogdandrutu merged commit caaa9a5 into open-telemetry:master Sep 8, 2020

Oberon00 deleted the exception-escaped branch September 8, 2020 14:44

This was referenced Sep 11, 2020

Error flagging with status codes open-telemetry/oteps#136

Merged

Exception semantic conventions: Add example & more explanation for exception.escaped. #946

Merged

This was referenced Sep 22, 2020

Add exception.escaped, clean up exception attributes javadoc. open-telemetry/opentelemetry-java#1672

Merged

RecordException and Span.Status #1008

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exception.escaped attribute. #784

Add exception.escaped attribute. #784

Oberon00 commented Aug 11, 2020 •

edited

Loading

arminru left a comment

bogdandrutu Aug 12, 2020

Oberon00 Aug 17, 2020 •

edited

Loading

Oberon00 Aug 17, 2020 •

edited

Loading

carlosalberto Aug 17, 2020

bogdandrutu Aug 17, 2020 •

edited

Loading

Oberon00 Aug 18, 2020

Oberon00 Aug 18, 2020

bogdandrutu Aug 18, 2020

Oberon00 Aug 18, 2020 •

edited

Loading

Oberon00 Aug 31, 2020

bogdandrutu left a comment

This comment has been minimized.

bogdandrutu commented Aug 30, 2020

Oberon00 commented Aug 30, 2020 •

edited

Loading

bogdandrutu commented Aug 31, 2020 •

edited

Loading

Oberon00 commented Sep 1, 2020 •

edited

Loading

bogdandrutu commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

Oberon00 commented Sep 2, 2020

Oberon00 commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

Oberon00 commented Sep 3, 2020

		- `RecordException(exception: Exception, escaped: boolean? = null)`
		- `RecordException(type: String, message: String, stacktrace: String, escaped: boolean?)`

Add exception.escaped attribute. #784

Add exception.escaped attribute. #784

Conversation

Oberon00 commented Aug 11, 2020 • edited Loading

Changes

arminru left a comment

Choose a reason for hiding this comment

bogdandrutu Aug 12, 2020

Choose a reason for hiding this comment

Oberon00 Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Oberon00 Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

carlosalberto Aug 17, 2020

Choose a reason for hiding this comment

bogdandrutu Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Oberon00 Aug 18, 2020

Choose a reason for hiding this comment

Oberon00 Aug 18, 2020

Choose a reason for hiding this comment

bogdandrutu Aug 18, 2020

Choose a reason for hiding this comment

Oberon00 Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Oberon00 Aug 31, 2020

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

This comment has been minimized.

bogdandrutu commented Aug 30, 2020

Oberon00 commented Aug 30, 2020 • edited Loading

bogdandrutu commented Aug 31, 2020 • edited Loading

Oberon00 commented Sep 1, 2020 • edited Loading

bogdandrutu commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

Oberon00 commented Sep 2, 2020

Oberon00 commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

bogdandrutu commented Sep 2, 2020

Oberon00 commented Sep 3, 2020

Oberon00 commented Aug 11, 2020 •

edited

Loading

Oberon00 Aug 17, 2020 •

edited

Loading

Oberon00 Aug 17, 2020 •

edited

Loading

bogdandrutu Aug 17, 2020 •

edited

Loading

Oberon00 Aug 18, 2020 •

edited

Loading

Oberon00 commented Aug 30, 2020 •

edited

Loading

bogdandrutu commented Aug 31, 2020 •

edited

Loading

Oberon00 commented Sep 1, 2020 •

edited

Loading