Lazily evaluate ExceptionEventData exception.* attributes #7172
Conversation
| | ||
| abstract SpanLimits getSpanLimits(); | ||
| | ||
| public abstract Attributes getAdditionalAttributes(); |
There was a problem hiding this comment.
cc @HaloFour I believe this should be a (temporary) workaround to the changes in #6795. To restore the behavior you were relying on, you should be able to:
- Check if
eventData instanceOf LazyExceptionEventData - If so, access the attributes via
LazyExceptionEventData#getAdditionalAttributes()without rendering theexception.*
I'd like to followup with this PRs change with:
- Add SDK customization hook for defining your own exception rendering logic
- Introduce API for setting exception on logs
LogRecordBuilder#setException - Improve the default exception rendering logic to be attribute limits aware, so that if attributes can't be longer than 1000 characters we don't waste resources rendering the stacktrace past this limit
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #7172 +/- ## ============================================ + Coverage 89.92% 89.94% +0.01% - Complexity 6721 6728 +7 ============================================ Files 765 766 +1 Lines 20277 20305 +28 Branches 1985 1988 +3 ============================================ + Hits 18234 18263 +29 - Misses 1448 1449 +1 + Partials 595 593 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/LazyExceptionEventData.java Outdated Show resolved Hide resolved
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/LazyExceptionEventData.java Outdated Show resolved Hide resolved
…y-java into lazy-evaluate-exception
| is there a connection or overlap between this PR and #7182? |
They solve separate problems but there will be merge conflicts I'll need to resolve regardless of which is merged first |
| Sounds good I'll review this one second |
…y-java into lazy-evaluate-exception
Should be good to go now that #7182 is merged |
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/LazyExceptionEventData.java Outdated Show resolved Hide resolved
| // - exception.type | ||
| // - exception.message | ||
| // - exception.stacktrace | ||
| return getAdditionalAttributes().size() + 3; |
There was a problem hiding this comment.
for users of this method does it matter that there could be overlap and so this calculation could overcount (e.g. a user overriding exception.message)?
There was a problem hiding this comment.
Oh great point. Need to think this through a little more
There was a problem hiding this comment.
should we move this to a Draft until you're ready to get this figured out?
3 participants
Currently, we compute
exception.*span event attributes immediately whenrecordExceptionis called.With this PR, those attributes are computed lazily at the time of export. This should shift CPU / memory from hot path to export background threads.