-
Notifications
You must be signed in to change notification settings - Fork 938
Have user supplied attributes take precedence over exception-derived attributes #7993
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7993 +/- ##
============================================
+ Coverage 89.99% 90.17% +0.18%
- Complexity 7079 7482 +403
============================================
Files 803 834 +31
Lines 21419 22564 +1145
Branches 2086 2240 +154
============================================
+ Hits 19276 20348 +1072
- Misses 1479 1514 +35
- Partials 664 702 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ((ExtendedLogRecordBuilder) logger.logRecordBuilder()) | ||
| .setException(new Exception("error")) | ||
| .setAttribute(EXCEPTION_MESSAGE, "custom message") | ||
| .emit(); |
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.
The user exception.message should take priority regardless of the invocation order yeah?
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.
| @@ -46,7 +45,7 @@ public ExtendedSdkLogRecordBuilder setException(Throwable throwable) { | |||
| loggerSharedState | |||
| .getExceptionAttributeResolver() | |||
| .setExceptionAttributes( | |||
| this::setAttribute, | |||
| new ExceptionAttributeSetterWithPrecedence(), | |||
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.
Could add a private method setExceptionAttribute (or similar) with the same signature as setAttribute. This would avoid allocating the extra ExceptionAttributeSetterWithPrecedence object. Not that its hugely important because we're in the setException code path which is allocation-heavy.
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.
Related to open-telemetry/opentelemetry-specification#4824 (comment)