-
Notifications
You must be signed in to change notification settings - Fork 889
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
Exception semantic conventions: Add example & more explanation for exception.escaped. #946
Exception semantic conventions: Add example & more explanation for exception.escaped. #946
Conversation
Co-authored-by: Tigran Najaryan <[email protected]>
Span span = myTracer.startSpan(/*...*/); | ||
try { | ||
// Code that does the actual work which the Span represents | ||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering whether we should hold up adding this example till the actual API change hits OTel Java ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosalberto OTel Java already supports this API. But the actual API has more boilerplate (I think I would have to write AttributeValue.booleanAttribute(true)
for the second parameter). See https://github.com/open-telemetry/opentelemetry-java/blob/v0.8.0/api/src/main/java/io/opentelemetry/trace/Span.java#L232-L239 (open-telemetry/opentelemetry-java#1599)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example is easier to read for non-Java users without that boilerplate, but I could add it too, if you prefer. Then I can write "Java" instead of "pseudo-Java" above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be personally more comfortable with the exact code we need (i.e. through Attribute
s). Else, go full pseudo code way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosalberto Now (d359f47) it's using the real OTel-Java API (at some version; seems like the Attributes are recently often changed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to get the last changes through before GA, but Attributes should settle down here quite soon. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please continue at #946 (comment)
// Code that does the actual work which the Span represents | ||
} catch (Throwable e) { | ||
span.recordException(e, Attributes.of( | ||
"exception.escaped", AttributeValue.booleanAttributeValue(true))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AttributeValue.booleanAttributeValue ? Could it not be just AttributeValue.of(true)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exception.escaped"
- in OpenTracing we usually had strongly typed constants for semantic conventions. E.g. something like this would be much more user friendly:
span.recordException(e, Attribute.Exception.Escaped(true))`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it should be possible to use (soon) SemanticAttributes.EXCEPTION_ESCAPED.set(span, true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro This example should be easy to read. Then @carlosalberto said in #946 (comment) I should use the actual Java API, so I added the boilerplate. Now you are saying the boilerplate looks verbose. Please decide this among you two, personally I would prefer readability over "real code" here. And for that matter, I think a literal string is far less confusing than a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosalberto This won't work for event attributes that way. I would probably have to call getKey(), wouldn't I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added f78003e to simplify the code again. It is now readable pseudo-Java and I prefer it that way.
This reverts commit d359f47.
Whoops, it seems I need to switch to YAML now 😅 Since I don't have Docker installed, this may take a while to get rebased... |
Conflicts: specification/trace/semantic_conventions/exceptions.md
Rebased and moved the new subsection to the "note" of exception.escaped. @carlosalberto @yurishkuro please check if the example is OK now. |
Co-authored-by: Armin Ruech <[email protected]>
See the [example above](#exception-end-example). It follows that an exception | ||
may still escape the scope of the span even if the `exception.escaped` attribute | ||
was not set or set to false, since that event might have been recorded at a time | ||
where it was not clear whether the exception will escape. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I find this paragraph exceedingly confusing. The first sentence says "difficult to say X, but trivial to say X" (X==X). The terms "in flight" and "escape" are undefined (and not a particularly common lingo). "in flight" exception, specifically, is not observable by the code, whereas a caught exception is not "in flight".
Terms that are not ambiguous: catch exception, re-throw exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These terms are not ambiguous but also not appropriate here. If you look at the code without the auto-instrumentation in place, the exception would indeed be currently in a state where it was thrown but not yet caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro I tried to clarify in 350ff89 although I think it would also be OK to be more vague here. WDYT?
…ception.escaped. (open-telemetry#946) * Add example & explanation for exception.escaped. * Reshuffle paragraphs. * Remove potentially controversial changes. * Fill in CHANGELOG link. * Address review comments. * Wording Co-authored-by: Tigran Najaryan <[email protected]> * Use actual OTel-Java API. * Revert "Use actual OTel-Java API." This reverts commit d359f47. * Clarify CHANGELOG * Re-generate tables. * Typo in CHANGELOG.md Co-authored-by: Armin Ruech <[email protected]> * Clarify exception escaped description. Co-authored-by: Tigran Najaryan <[email protected]> Co-authored-by: Armin Ruech <[email protected]>
Follow up for #784 that recovers parts of #761. No semantic changes intended, just adds an example (with link from semantic conventions to recordException API) and an expanded explanation.