Specify which exceptions SHOULD be recorded

[Oberon00]

Replaces #521 "Semantic conventions for when to record exceptions.

Issues that came up in this PR but should be handled separately:

• Exception semantic conventions and Go error return values #764 "Exception semantic conventions and Go error return values"

Change summary

• State that exceptions that leave the scope of a Span SHOULD be recorded.

Major changes this PR underwent

• This PR originally proposed that exceptions other than those that leave the scope of a span MUST NOT be recorded. This was replaced by adding the exception.left_scope attribute, now renamed to exception.escaped. Commit: 11f0543; Discussion: Specify which exceptions SHOULD be recorded #761 (comment)
• The exception.escaped (aka exception.left_scope) attribute was removed from this PR into Add exception.escaped attribute. #784 because it is not as important and more controversial than the rest of this PR. Commit: 80ea27d; Discussion: SIG meeting #61

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Christian Neumüller (7da8f56, 1f32e2c, 1354f86, a20bea9, 11f0543)

[bogdandrutu]

I would vote to not merge this PR:

• This PR without clearly stated adds a notion of "back" propagation which we haven't defined yet and there may be other informations that we may need to propagate not only exceptions.
• Unclear what do we try to achieve with the special "left_scope" (also name is bad, initially I thought about where is the "right_scope", probably "left_the_scope").
• Why do we not say that exception should be recorded in every Span that "observes" it. Which means:
  • ParentSpan -> ChildSpan
  • ChildSpan throws exception, and records it, does not handle the exception
  • ParentSpan catches that exception (may throw, or handle) then it also records the exception.

[Oberon00]

This PR without clearly stated adds a notion of "back" propagation which we haven't defined yet

Actually no. In fact I tried very hard to make clear that no back back propagation is needed at all. That's why added

Note that multiple events (on the same or different Spans) might be logged for the same exception object instance.

and also

SHOULD be set to true if the exception event is recorded while observing the exception leaving the scope of the span.

If you tell me what makes you think of back propagation here, I will try to reword the spec to make it more clear.

Unclear what do we try to achieve with the special "left_scope" (also name is bad, initially I thought about where is the "right_scope", probably "left_the_scope").

It is a piece of information that can be used to determine which exception is likely to have been a cause for failure. Also it's very easy to implement this in instrumentations (no back propagation needed, you just will have a literal true if you implement your instrumentation in certain ways and leave it unset if you implement it in other ways).

Why do we not say that exception should be recorded in every Span that "observes" it.

But we do?

An unhandled exception that leaves the scope of a span SHOULD be recorded as an Event on that span.

(actually the "unhandled" is redundant here, I will remove it).

We have no SHOULD requirement for a span that catches an exception which occurs somewhere within it, since such exceptions typically go unobserved (you would have to instrument every catch block, which would generate noise for catch blocks that only do some cleanup and then rethrow).

[Oberon00]

As discussed in the SIG meeting today, removed the new semantic attribute (will submit it as a separate PR; EDIT: #784) and changed after-ga to required-for-ga for the remainder.

CC @bogdandrutu

[bogdandrutu]

Should we do this? Can we avoid it?

[Oberon00]

Hmm, it does add information about the path of the exception although the full stacktrace would not be relevant each time. I don't think this can be easily avoided.

[bogdandrutu]

Hmm.. is the stacktrace not captured when the exception is generated (at least in some languages)? Also what is the point of having stacktrace twice?

[Oberon00]

There is no point, but you usually dont't know if the exception was already/will again be recorded.

Maybe a clever exporter can do de-duplication.

[Oberon00]

Note that this is a "Note" stating what I think can happen in practice.

To allow improving this situation, we could do something like assign an ID to each exception when it is first observed and only record the ID for each further occurrence. But that's probably an after-ga thing.

[tigrannajaryan]

@Oberon00 it is not entirely clear who these recommendations are targeted to. Is it for OpenTelemetry SDK developers or for application developers who use the OpenTelemetry in their apps? Can you please clarify the intent?

[Oberon00]

@tigrannajaryan
At this point, recommendation in semantic conventions are always directed "to whom it may concern".

In languages that only support recordException APIs, this will be something that has to be done by the user (whoever calls these APIs), although SDK implementors should probably mention this spec part in their documentation for this method.

In other languages, where the API can provide more convenient support for this (for example Python already has a context manager for Spans that automatically sets the status code if an exception is thrown, see https://github.com/open-telemetry/opentelemetry-python/blob/b3c2a0372e27eff4594891db8ad04c438e9fdf4c/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L677), the SDK+API may also help implement this SHOULD requirement.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[Oberon00]

@murphpdx You originially proposed this in #521. If you are still interested, please speak up, otherwise it seems to me that interest in this spec is too low. Personally, I also don't think it is too important to have this in the spec (although I do think it would be somewhat useful and IIRC @bogdandrutu did say in the SIG meeting that we should specify this before release)

I would merge the definitions of "escaped" plus the example into/after #784, if this goes stale.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[Oberon00]

Closing this, because there does not seem to be enough interest.