Stabilize ExceptionEventData

[vasantteja]

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]

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

[vasantteja]

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

[jkwatson]

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

[codecov]

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.

[jkwatson]

Thanks!

[jack-berg]

Few discussion points

[jack-berg]

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

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?

[jkwatson]

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

[jack-berg]

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.

[jack-berg]

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

[jkwatson]

I'm totally ok with your suggestion

[vasantteja]

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

[jack-berg]

You've got it.

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

[vasantteja]

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

things change

[jack-berg]

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]

👍