-
Notifications
You must be signed in to change notification settings - Fork 882
Configurable exception.* attribute resolution #7266
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
base: main
Are you sure you want to change the base?
Configurable exception.* attribute resolution #7266
Conversation
…y-java into logbuilder-set-exception
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7266 +/- ##
============================================
- Coverage 89.62% 89.61% -0.02%
- Complexity 6864 6873 +9
============================================
Files 780 782 +2
Lines 20731 20772 +41
Branches 2023 2023
============================================
+ Hits 18581 18614 +33
- Misses 1513 1518 +5
- Partials 637 640 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…y-java into exception-attribute-resolver
* to avoid unnecessary compute. | ||
*/ | ||
@Nullable | ||
String getExceptionType(Throwable throwable, int maxAttributeLength); |
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.
Realized that for alternative more perform implementations of exception.stacktrace
to be possible, they need to know the attribute length limit so they can exit early.
The limit is much less relevant for exception.type
and exception.message
, but seemed more consistent to include it everywhere even if its likely not needed.
I also considered a solution where this is an abstract class instead of an interface, and has accepts max attribute length as a constructor parameter which is accessible by implementations. But in practice, max attribute length comes from SpanLimits, which can be re-configured at runtime by setting Supplier<SpanLimit>
. And so an abstract class design would likely mean re-initializing ExceptionAttributeResolver for each span to ensure we get the up to date limits, which is wasteful.
Better to include it as parameter in the API, despite it making the API appear a little cluttered.
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.
An alternative API design would be to have a single method responsible for computing and setting all the attributes:
<T> void addExceptionAttributes(BiConsumer<AttributeKey<T>, T> attributeSetter, Throwable, int maxAttributeLength)
This is a cleaner API, and allows users to attach the exception using entirely different conventions. For example, maybe they want to record the exception stacktrace as a array of strings instead of as a string. This makes it easier to break semantic conventions, and also doesn't allow for different exception attributes to be computed at different stages of a span's lifecycle. For example, in #7172 I'm proposing lazily computing exception.stacktrace
and exception.type
on the export path, and eagerly computing exception.message
on the hot path.
Resolves #5187.
Make
exception.*
resolution configurable. With #7182,exception.*
attributes are now resolved and attached to both span events and log records. This is an edge cases where the SDK specification has a dependency on the semantic conventions. It seems appropriate to make this a configurable extension point, so that users can customize things like theexception.stacktrace
resolution, and whether they even want exception attributes included at all.Broken out from #7182. Leaving as a draft because this makes the most sense after merging #7182.