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

Jump to bottom

Stabilize ExceptionEventData #6795

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+14 −27
Open

Stabilize ExceptionEventData #6795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 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
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. 🤔

+++ 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
12 changes: 4 additions & 8 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,18 +3,14 @@
* 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;
import javax.annotation.concurrent.Immutable;

/**
* Data representation of an event for a recorded exception.
*
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
*/
/** Data representation of an event for a recorded exception. */
vasantteja marked this conversation as resolved.
Show resolved Hide resolved
@Immutable
public interface ExceptionEventData extends EventData {

/**
Expand Down
2 changes: 1 addition & 1 deletion ...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 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