Adds option to SpanLimits to exclude exception.stacktrace attribute #7133
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7133 +/- ##
============================================
+ Coverage 89.85% 89.86% +0.01%
- Complexity 6610 6614 +4
============================================
Files 740 740
Lines 19986 19993 +7
Branches 1964 1965 +1
============================================
+ Hits 17958 17967 +9
Misses 1439 1439
+ Partials 589 587 -2 ☔ View full report in Codecov by Sentry. |
| === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 | ||
| +++ NEW METHOD: PUBLIC(+) boolean isExcludeExceptionStackTrace() | ||
| *** MODIFIED CLASS: PUBLIC FINAL io.opentelemetry.sdk.trace.SpanLimitsBuilder (not serializable) | ||
| === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 | ||
| +++ NEW METHOD: PUBLIC(+) io.opentelemetry.sdk.trace.SpanLimitsBuilder setExcludeExceptionStackTrace(boolean) |
There was a problem hiding this comment.
it may be worth considering the future direction proposed in open-telemetry/opentelemetry-specification#4333 before adding public API surface
There was a problem hiding this comment.
Thank you for that link. I will review and consider how it impacts this PR.
There was a problem hiding this comment.
There's a lot going on in that discussion. It sounds like even if the recommendation is that instrumentation emit exceptions as log events that there should still be some form of configuration to bridge that back to span events.
While it seems like a lot of things are up in the air have there been any discussions as to how that OTEP might impact the Java SDK and instrumentation?
Either way I think there's value in having some degree of control over how exceptions are interpreted and rendered into telemetry signals, including spans, so I would like to continue the conversation as to how that could be exposed in the SDK.
There was a problem hiding this comment.
Either way I think there's value in having some degree of control over how exceptions are interpreted and rendered into telemetry signals
I agree. With logs / events right now, instrumentation is responsible for this. See how the logback instrumentation has the same SDK exception span event rendering logic embedded in it. When its an instrumentation concern, that means that every instrumentation has to handle it separately. In theory that means that there's a lot of control over the rendering, but in practice, there's really only control if the instrumentation libraries expose configuration options to control the rendering. So it becomes a decentralized problem instead of a problem centrally handled / configured in the SDK. Ouch.
@trask I wonder if we should add a method to LogRecordBuilder which allows users to attach an exception directly to the log record. Then, we could have a log SDK configuration option for configuring rendering which is consistent / symmetric with rendering configuration in the trace SDK.
There was a problem hiding this comment.
That's interesting to me that the OTel Log instrumentation for JUL, log4j and logback simply emit the entire stacktrace rather than using the formatting facilities that are provided. It definitely feels like this might be something that the OTel SDK should provide and that instrumentation should be abstracted away from those concerns to keep the behavior consistent and user-configurable. I'd be happy to help work on such a project.
There was a problem hiding this comment.
@jack-berg since you're hopefully not trying to follow all of the logging discussions, here's the latest thought for attaching a terminal exception directly to a span: open-telemetry/opentelemetry-specification#4429 (comment)
There was a problem hiding this comment.
here's the latest thought for attaching a terminal exception directly to a span
👍 this also seems to align with @HaloFour's thought. Of course, all of this will have to be configurable given the fact that folks may be depending on the current stable behavior.
There was a problem hiding this comment.
folks may be depending on the current stable behavior
the general strategy we're proposing in the spec:
- no changes to any existing SDK behavior
- introduce new API with new behavior
- deprecate old API to signal that new code should use the new API
- ask (at least stable) instrumentations to bump their major version when they move from the old API to the new API
There was a problem hiding this comment.
ask (at least stable) instrumentations to bump their major version when they move from the old API to the new API
Could that be an abstraction provided by the instrumentation API itself so that individual instrumentations don't need to be aware of how exceptions end up getting emitted?
There was a problem hiding this comment.
👍 if you're specifically referring to io.opentelemetry.instrumentation:opentelemetry-instrumentation-api
Adds an option on
SpanLimitswhich specifies that theexception.stacktraceattribute should not be emitted on exception events recorded to a span.SpanLimitsmight not the right place to put this, but I wanted to get something out there for the purposes of discussion.