Skip to content
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.

Sign up for GitHub

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

Stabilize ExceptionEventData #6795

Merged
merged 14 commits into from
Oct 29, 2024
Merged

Stabilize ExceptionEventData #6795

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
219ae6f
wip: stabilizing event exception data.
vasantteja Oct 15, 2024
e7c0a03
polish: adding api diffs.
vasantteja Oct 16, 2024
015a707
polish: removing the fully qualified class names and refining the cod…
vasantteja Oct 16, 2024
b6e5816
polish: adding a missing comment.
vasantteja Oct 16, 2024
9700e01
polish: Update the comment around the class.
vasantteja Oct 22, 2024
661fc17
polish: applying spotless checks.
vasantteja Oct 22, 2024
b89292a
polish: removing additional attributes and converting ExceptionEventD…
vasantteja Oct 27, 2024
7cc7162
polish: removing the unwanted initialization for attributes.
vasantteja Oct 27, 2024
e5e686c
polish: static instantiation.
vasantteja Oct 27, 2024
131062a
Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…
vasantteja Oct 28, 2024
b084f01
Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…
vasantteja Oct 28, 2024
d2e0cea
Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
vasantteja Oct 28, 2024
aefe947
polish: text fixtures and code refinement.
vasantteja Oct 29, 2024
18c2578
Capture exceptin event total added attribute values, remove unused me…
jack-berg Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
Comparing source compatibility of opentelemetry-sdk-trace-1.44.0-SNAPSHOT.jar against opentelemetry-sdk-trace-1.43.0.jar
No changes.
+++ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

+++ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally ok with your suggestion

Copy link
Contributor Author

@vasantteja vasantteja Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-berg I understand that we should remove getAdditionalAttributes() and SpanLimits from the implementation. Your suggestion to make this similar to EventData makes sense to me. The reason behind this is that the caller can retrieve the additional attributes using this call getAttributes().toBuilder().removeIf(attributeKey -> attributeKey.getKey().startsWith("exception")).build();. Did I get this right, or am I missing anything? If yes, I can make the changes suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've got it.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

+++ NEW METHOD: PUBLIC(+) ABSTRACT(+) io.opentelemetry.api.common.Attributes getAdditionalAttributes()
+++ NEW METHOD: PUBLIC(+) ABSTRACT(+) java.lang.Throwable getException()
2 changes: 1 addition & 1 deletion sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import io.opentelemetry.sdk.internal.InstrumentationScopeUtil;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.data.EventData;
import io.opentelemetry.sdk.trace.data.ExceptionEventData;
import io.opentelemetry.sdk.trace.data.LinkData;
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.sdk.trace.data.StatusData;
import io.opentelemetry.sdk.trace.internal.ExtendedSpanProcessor;
import io.opentelemetry.sdk.trace.internal.data.ExceptionEventData;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down
27 changes: 16 additions & 11 deletions ...ace/internal/data/ExceptionEventData.java → ...ry/sdk/trace/data/ExceptionEventData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.trace.internal.data;
package io.opentelemetry.sdk.trace.data;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.trace.SpanLimits;
import io.opentelemetry.sdk.trace.data.EventData;

/**
* Data representation of an event for a recorded exception.
Expand All @@ -18,13 +17,15 @@
public interface ExceptionEventData extends EventData {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @param spanLimits limits applied to {@link ExceptionEventData}.
* @param epochNanos epoch timestamp in nanos of the {@link ExceptionEventData}.
* @param spanLimits limits applied to {@link io.opentelemetry.sdk.trace.data.ExceptionEventData}.
* @param epochNanos epoch timestamp in nanos of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}.
* @param exception the {@link Throwable exception} of the {@code Event}.
* @param additionalAttributes the additional attributes of the {@link ExceptionEventData}.
* @return a new immutable {@link ExceptionEventData}
* @param additionalAttributes the additional attributes of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}.
* @return a new immutable {@link io.opentelemetry.sdk.trace.data.ExceptionEventData}
*/
static ExceptionEventData create(
SpanLimits spanLimits,
Expand All @@ -36,16 +37,20 @@ static ExceptionEventData create(
}

/**
* Return the {@link Throwable exception} of the {@link ExceptionEventData}.
* Return the {@link Throwable exception} of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}.
*
* @return the {@link Throwable exception} of the {@link ExceptionEventData}
* @return the {@link Throwable exception} of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}
*/
Throwable getException();

/**
* Return the additional {@link Attributes attributes} of the {@link ExceptionEventData}.
* Return the additional {@link Attributes attributes} of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}.
*
* @return the additional {@link Attributes attributes} of the {@link ExceptionEventData}
* @return the additional {@link Attributes attributes} of the {@link
* io.opentelemetry.sdk.trace.data.ExceptionEventData}
*/
Attributes getAdditionalAttributes();
}
6 changes: 3 additions & 3 deletions ...nal/data/ImmutableExceptionEventData.java → ...ace/data/ImmutableExceptionEventData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.trace.internal.data;
package io.opentelemetry.sdk.trace.data;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
Expand All @@ -16,10 +16,10 @@
import java.io.StringWriter;
import javax.annotation.concurrent.Immutable;

/** An effectively immutable implementation of {@link ExceptionEventData}. */
@AutoValue
@Immutable
abstract class ImmutableExceptionEventData implements ExceptionEventData {
abstract class ImmutableExceptionEventData
implements io.opentelemetry.sdk.trace.data.ExceptionEventData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static final AttributeKey<String> EXCEPTION_TYPE =
AttributeKey.stringKey("exception.type");
Expand Down
15 changes: 0 additions & 15 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/package-info.java
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.time.TestClock;
import io.opentelemetry.sdk.trace.data.EventData;
import io.opentelemetry.sdk.trace.data.ExceptionEventData;
import io.opentelemetry.sdk.trace.data.LinkData;
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.sdk.trace.data.StatusData;
import io.opentelemetry.sdk.trace.internal.ExtendedSpanProcessor;
import io.opentelemetry.sdk.trace.internal.data.ExceptionEventData;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.time.Duration;
Expand Down
Loading