Configurable exception.* attribute resolution#7266
Configurable exception.* attribute resolution#7266jack-berg merged 11 commits intoopen-telemetry:mainfrom
Conversation
…y-java into logbuilder-set-exception
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7266 +/- ##
============================================
- Coverage 89.83% 89.75% -0.08%
- Complexity 6895 6977 +82
============================================
Files 786 797 +11
Lines 20793 21165 +372
Branches 2026 2056 +30
============================================
+ Hits 18679 18997 +318
- Misses 1469 1505 +36
- Partials 645 663 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…y-java into exception-attribute-resolver
sdk/common/src/main/java/io/opentelemetry/sdk/internal/ExceptionAttributeResolver.java
Outdated
Show resolved
Hide resolved
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/internal/SdkLoggerProviderUtil.java
Show resolved
Hide resolved
|
cc @HaloFour |
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
Outdated
Show resolved
Hide resolved
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
Outdated
Show resolved
Hide resolved
|
Alright I've refactored this to reflect the recommendation from this convo to consolidate into a single method. Please take another look when you can @trask! Would love to get this merged to unblock #7281, which I think will make a nice positive perf impact for many users. |
sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkTracerProviderTest.java
Show resolved
Hide resolved
| public Object put(AttributeKey<?> key, Object value) { | ||
| public Object put(AttributeKey<?> key, @Nullable Object value) { | ||
| if (value == null) { | ||
| return null; |
There was a problem hiding this comment.
does this change have any effect on existing behaviors that we should be aware of?
There was a problem hiding this comment.
No it shouldn't.
I wasn't quite remembering why I added this in the first place, so I retraced my steps and determined:
ExceptionAttributeResolver.AttributeSetter#setAttributeaccepts a@Nullable T value. We want this because we recently updated Span, LogRecord setAttribute methods to accept@Nullablevalues.- AttributeMap#putIfCapacity acts as the implementation for this method
- AttributeMap#putIfCapacity delegates to AttributeMap#put (I want both methods to reduce accidental mistakes of bypassing the validation, as discussed in the javadoc of this class)
|
Hey Folks, as a followup to this PR, is there work planned to make the log4j / logback appenders use the |
|
hi @philsttr, can you open an issue in the java instrumentation repo where those appenders live? thanks! |
Part of [OTEP 4430](https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/4430-span-event-api-deprecation-plan.md). This could be considered "just a convenience", but (in addition to a very nice convenience) it also unlocks two additional features: - Performance optimization since the SDK can take `OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT` into consideration when generating large `exception.stacktrace` attributes (a very real issue in the Java world: open-telemetry/opentelemetry-java#7281) - (Future) customization of exception stacktraces (we have experimental support for this in Java, e.g. to support "smarter" truncation of long `exception.stacktrace` attributes: open-telemetry/opentelemetry-java#7266) Prototype: open-telemetry/opentelemetry-java#7182 --------- Co-authored-by: Robert Pająk <[email protected]>
5 participants
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.stacktraceresolution, 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.