Stabilize ExceptionEventData #6795

vasantteja · 2024-10-16T01:38:07Z

This Change

I promoted both the interface and implementation to data package adhering to the package structure inside the internal implementation. Let me know your thoughts. Sorry for opening another pr when we have another pr. This resolves 6630

jkwatson · 2024-10-16T01:44:56Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java


 /**
- * Returns a new immutable {@link ExceptionEventData}.
+ * Returns a new immutable {@link io.opentelemetry.sdk.trace.data.ExceptionEventData}.


this shouldn't require being fully qualified, I don't think. I'd revert the fully-qualified classname changes here.

Dang it, sorry I will just use the class name and remove the fully-qualified classname.

jkwatson · 2024-10-16T01:45:14Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ImmutableExceptionEventData.java

 @Immutable
-abstract class ImmutableExceptionEventData implements ExceptionEventData {
+abstract class ImmutableExceptionEventData
+ implements io.opentelemetry.sdk.trace.data.ExceptionEventData {


this should be importable, and not need to be fully-qualified

codecov · 2024-10-16T01:49:20Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.49%. Comparing base (b1cd30e) to head (18c2578).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	92.30%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@ ## main #6795 +/- ## ============================================ + Coverage 90.12% 90.49% +0.37%  - Complexity 6546 6598 +52  ============================================ Files 728 731 +3 Lines 19702 19735 +33 Branches 1935 1938 +3 ============================================ + Hits 17756 17859 +103  + Misses 1347 1283 -64  + Partials 599 593 -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…e a little for removing misleading comments from copy paste.

jkwatson

Thanks!

jack-berg

Few discussion points

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java

jack-berg · 2024-10-21T15:21:55Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+++ CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+++ NEW INTERFACE: io.opentelemetry.sdk.trace.data.EventData
+++ NEW SUPERCLASS: java.lang.Object
+++ NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(io.opentelemetry.sdk.trace.SpanLimits, long, java.lang.Throwable, io.opentelemetry.api.common.Attributes)


@jkwatson its odd for the create method to accept SpanLimits as an argument. Normally the application of limits is handled in SDK internals. We can see this pattern play out in the EventData.create contract:

static EventData create( long epochNanos, String name, Attributes attributes, int totalAttributeCount)

The attributes are provided with an additional totalAttributeCount indicating how many attributes were originally present.

The reason we do it differently for ExceptionEventData appears to be because ImmutableExceptionEvent#getAttributes() doesn't compute the attributes until it is called (and then it memoizes them), preventing additional allocations on the hot path.

Not sure there's a way to avoid SpanLimits being passed as an argument if we stick with that argument.

jack-berg · 2024-10-21T15:27:57Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+++ NEW INTERFACE: io.opentelemetry.sdk.trace.data.EventData
+++ NEW SUPERCLASS: java.lang.Object
+++ NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(io.opentelemetry.sdk.trace.SpanLimits, long, java.lang.Throwable, io.opentelemetry.api.common.Attributes)
+++ NEW METHOD: PUBLIC(+) ABSTRACT(+) io.opentelemetry.api.common.Attributes getAdditionalAttributes()


Upon closer inspection, I find this method strange. The resulting ExceptionEventData interface has the following methods:

getAttributes(): default implementation returns getAdditionalAttributes() provided by user + exception.* attributes following semconv.

getAdditionalAttributes(): default implementation returns only additional attributes provided by the user.

Seems odd to have a special accessor for getAdditionalAttributes(). I assume that consumers of this want to do something like: if(event instanceOf ExceptionEventData) serialize(((ExceptionEventData) event).getException())

If they want o split out the exception attributes from additional attributes, can do so with getAttributes().toBuilder().removeIf(attributeKey -> attributeKey.getKey().startsWith("exception")).build().

Do we really need the special getAdditionalAttributes() accessor?

hmm. good eye on double checking the original API for this. Maybe we're not quite ready to stabilize, if we're unsure of the final API? I agree the creational patterns for this are currently strange and not user-friendly. 🤔

I think we can still stabilize, but cut out the getAdditionalAttributes() method. The point I make here about SpanLimits as a constructor argument is an implementation detail. We can remove it as a parameter with the only difference being that we compute the exception.* attributes on the calling thread instead of on a background thread, which seems pretty reasonable / expected.

If you agree, I can either push some commits to this PR, or provide some more complete guidance to @vasantteja on how to proceed.

I'm totally ok with your suggestion

You've got it.

The exception.* attributes will need to be computed in SdkSpan, rather than in ImmutableExceptionEventData.

@jack-berg I finally wrapped my head around this and have an implementation. I cut out the attribute computation in the ImmutableExceptionEventData and I moved them to SdkSpan. I added a little bit of logic to shrink the exception messages if maxLength attribute is turned on in SpanLimits. Let me know if this aligns with your thinking.

I wish I had noticed this PR earlier. Unfortunately moving the rendering of the exception back into SdkSpan makes it unavoidable and doing so is incredibly expensive. In our Spring WebFlux services not only are the stack traces absolutely massive, but the rendering of the traceback creates resource contention due to synchronization. It causes a noticeable impact on the performance of our services. I would like to revisit this decision and at least provide a means to make it optional.

@HaloFour I'd suggest opening a new issue where this can be discussed, thanks!

Opened this issue: #7129

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

things change

…ata to align with EventData.

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ImmutableExceptionEventData.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

…bleExceptionEventData.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

…thods

jack-berg · 2024-10-29T14:09:19Z

I pushed a small commit to capture the total number of attributes added (in case any were filtered to conform to SpanLimits), and to remove unused methods. Looks good to merge to me!

trask · 2024-10-29T18:28:55Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+++ NEW INTERFACE: PUBLIC(+) ABSTRACT(+) io.opentelemetry.sdk.trace.data.ExceptionEventData (not serializable)
+++ CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+++ NEW INTERFACE: io.opentelemetry.sdk.trace.data.EventData
+++ NEW SUPERCLASS: java.lang.Object
+++ NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(long, java.lang.Throwable, io.opentelemetry.api.common.Attributes, int)
+++ NEW METHOD: PUBLIC(+) ABSTRACT(+) java.lang.Throwable getException()


vasantteja added 2 commits October 15, 2024 08:54

wip: stabilizing event exception data.

219ae6f

polish: adding api diffs.

e7c0a03

vasantteja requested a review from a team as a code owner October 16, 2024 01:38

jkwatson reviewed Oct 16, 2024

View reviewed changes

vasantteja added 2 commits October 15, 2024 21:59

polish: removing the fully qualified class names and refining the cod…

015a707

…e a little for removing misleading comments from copy paste.

polish: adding a missing comment.

b6e5816

jkwatson previously approved these changes Oct 16, 2024

View reviewed changes

jack-berg reviewed Oct 21, 2024

View reviewed changes

vasantteja and others added 2 commits October 21, 2024 21:33

polish: Update the comment around the class.

9700e01

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

polish: applying spotless checks.

661fc17

jkwatson self-requested a review October 22, 2024 20:10

vasantteja added 2 commits October 27, 2024 17:34

polish: removing additional attributes and converting ExceptionEventD…

b89292a

…ata to align with EventData.

polish: removing the unwanted initialization for attributes.

7cc7162

jkwatson reviewed Oct 27, 2024

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java Outdated Show resolved Hide resolved

polish: static instantiation.

e5e686c

jack-berg reviewed Oct 28, 2024

View reviewed changes

vasantteja and others added 5 commits October 28, 2024 18:58

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…

131062a

…bleExceptionEventData.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…

b084f01

…bleExceptionEventData.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

d2e0cea

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

polish: text fixtures and code refinement.

aefe947

Capture exceptin event total added attribute values, remove unused me…

18c2578

…thods

jack-berg approved these changes Oct 29, 2024

View reviewed changes

jack-berg merged commit fcae15e into open-telemetry:main Oct 29, 2024
20 checks passed

trask reviewed Oct 29, 2024

View reviewed changes

jack-berg mentioned this pull request Feb 20, 2025

Moving the rendering of the exception back into SdkSpan makes exception attribute rendering unavoidable and is expensive #7129

Open

jack-berg mentioned this pull request Mar 4, 2025

Lazily evaluate ExceptionEventData exception.* attributes #7172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stabilize ExceptionEventData #6795

Stabilize ExceptionEventData #6795

Uh oh!

vasantteja commented Oct 16, 2024

jkwatson Oct 16, 2024

vasantteja Oct 16, 2024

jkwatson Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

jkwatson left a comment

jack-berg left a comment

Uh oh!

jack-berg Oct 21, 2024

jack-berg Oct 21, 2024

jkwatson Oct 21, 2024

jack-berg Oct 23, 2024

jack-berg Oct 23, 2024

jkwatson Oct 23, 2024

jack-berg Oct 24, 2024

vasantteja Oct 27, 2024

HaloFour Feb 20, 2025

trask Feb 20, 2025

jack-berg Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jack-berg commented Oct 29, 2024

Uh oh!

trask Oct 29, 2024

Labels

5 participants

Stabilize ExceptionEventData #6795

Stabilize ExceptionEventData #6795

Uh oh!

Conversation

vasantteja commented Oct 16, 2024

This Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

jkwatson left a comment

Choose a reason for hiding this comment

jack-berg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jack-berg commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Labels

5 participants

codecov bot commented Oct 16, 2024 •

edited

Loading