Stabilize ExceptionEventData #6795
Conversation
| @@ -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}. | |||
There was a problem hiding this comment.
this shouldn't require being fully qualified, I don't think. I'd revert the fully-qualified classname changes here.
There was a problem hiding this comment.
Dang it, sorry I will just use the class name and remove the fully-qualified classname.
| @AutoValue | ||
| @Immutable | ||
| abstract class ImmutableExceptionEventData implements ExceptionEventData { | ||
| abstract class ImmutableExceptionEventData | ||
| implements io.opentelemetry.sdk.trace.data.ExceptionEventData { |
There was a problem hiding this comment.
this should be importable, and not need to be fully-qualified
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6795 +/- ##
============================================
+ Coverage 90.12% 90.38% +0.25%
- Complexity 6546 6577 +31
============================================
Files 728 731 +3
Lines 19702 19714 +12
Branches 1935 1926 -9
============================================
+ Hits 17756 17818 +62
+ Misses 1347 1300 -47
+ Partials 599 596 -3 ☔ View full report in Codecov by Sentry. |
…e a little for removing misleading comments from copy paste.
sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java
Show resolved
Hide resolved
| +++ 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) |
There was a problem hiding this comment.
@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 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() |
There was a problem hiding this comment.
Upon closer inspection, I find this method strange. The resulting ExceptionEventData interface has the following methods:
getAttributes(): default implementation returnsgetAdditionalAttributes()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?
There was a problem hiding this comment.
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. 🤔
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
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