Added SpanProcessor OnEnding callback

[JonasKunz]

Implementation for the spec change open-telemetry/opentelemetry-specification#4024.

[codecov]

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.01%. Comparing base (3e8092d) to head (80aa059).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6367      +/-   ##
============================================
- Coverage     90.05%   90.01%   -0.05%     
- Complexity     6276     6330      +54     
============================================
  Files           697      703       +6     
  Lines         18949    19086     +137     
  Branches       1858     1881      +23     
============================================
+ Hits          17065    17180     +115     
- Misses         1312     1331      +19     
- Partials        572      575       +3     

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

[jkwatson]

I think it's probably a mistake to be calling this under the lock. I believe we only want to do internal state management while the lock is being held, not call not-under-our-control methods provided by users.

[JonasKunz]

The lock was actually intentionally hold to cover this part of the spec:

The SDK MUST guarantee that the span can no longer be modified by any other thread
before invoking OnEnding of the first SpanProcessor. From that point on, modifications
are only allowed synchronously from within the invoked OnEnding callbacks.

This part was added to the spec so that SpanProcessors can be sure that the state they see the span in is actually consistent and not prone to race conditions due to span modifications from the application code.

However, I now switched to a different way of implementing this, because as you said exposing the lock is probably not the best way of achieving the desired safety here.

[jack-berg]

Couple minor comments, but seems generally good. Thanks!

[jkwatson]

I approve of putting this in a separate interface. But, we should provide documentation on how someone could use this, as well.

[JonasKunz]

But, we should provide documentation on how someone could use this, as well.

What do you think would be the best place for this? Just a javadoc in ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

[jack-berg]

What do you think would be the best place for this? Just a javadoc in ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

I think the ideal way to do this is a unit test. Let's add an ExtendedSpanProcessorUsageTest akin to what we do in the api incubator. No obvious place to link to the usage test at the moment, but let's start by at least demonstrating its usage in clear terms.

[jack-berg]

thanks for adding the example. couple of nits but ready otherwise ready to merge IMO