Implement span processor's OnEnding

[dmathieu]

This allows span processors to implement the new, in development specification, OnEnding.

Spec PR: open-telemetry/opentelemetry-specification#4024

pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Apple M1 Max
           │ bench-main  │         bench-branch          │
           │   sec/op    │   sec/op     vs base          │
SpanEnd-10   151.9n ± 9%   154.6n ± 8%  ~ (p=0.684 n=10)

           │ bench-main │          bench-branch          │
           │    B/op    │    B/op     vs base            │
SpanEnd-10   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

           │ bench-main │          bench-branch          │
           │ allocs/op  │ allocs/op   vs base            │
SpanEnd-10   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (be328ac) to head (f4ae0f4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5756   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22734   22746   +12     
=====================================
+ Hits       19226   19238   +12     
  Misses      3165    3165           
  Partials     343     343           

see 1 file with indirect coverage changes

[MrAlias]

This PR looks fine in general. However, this is adding a non-stable portion of the specification into this stable package. For that reason I'm blocking this PR. We do not want to add something we do not plan to support in the long term.

[dmathieu]

Would you rather the interface be in sdk/internal/x?

[MrAlias]

Would you rather the interface be in sdk/internal/x?

SGTM

[dashpole]

SGTM as well.

[pellared]

Could you make the docs better? Look at #5692 for reference.

[dmathieu]

@open-telemetry/go-approvers ping for review?

[dmathieu]

@MrAlias since you requested changes, can you review/approve this PR?

[MrAlias]

This acquires the lock twice. Can this be avoided instead?

[dmathieu]

Not really, as we need to set the end time, and later on, mark the span as having ended.

[MrAlias]

This is going to impact the performance of users who are not using the experimental feature. We need to look into alternatives.

[dmathieu]

I've changed to only lock once if there are no onending processors.

[MrAlias]

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 does not prevent the span from being modified by any other goroutine that holds a reference to the span.

[dmathieu]

I'm open to ideas about that.
@pellared wondered about this too: https://github.com/open-telemetry/opentelemetry-specification/pull/4024/files#r1656569090
However, the proposal to acquire a lock while we execute OnEnding doesn't seem like it would work. It would prevent modifications of the span within the processor, since they would wait for the lock too.

There are two other implementations so far:

• Java checks that we are in the same thread, which we can't do in Go: https://github.com/open-telemetry/opentelemetry-java/pull/6367/files#diff-d716139d47f651d6b6558f3e6f03aa59c98e52723f472b2c6a24bd02ec5fe2ceR340
• Ruby doesn't handle that at all.

[pellared]

Not compliant with the spec per #5756 (comment). Maybe the spec should be revisited? If I understand correctly then this proposal is also not compliant with the spec.

[MrAlias]

I'm open to ideas about that. @pellared wondered about this too: https://github.com/open-telemetry/opentelemetry-specification/pull/4024/files#r1656569090 However, the proposal to acquire a lock while we execute OnEnding doesn't seem like it would work. It would prevent modifications of the span within the processor, since they would wait for the lock too.

There are two other implementations so far:

• Java checks that we are in the same thread, which we can't do in Go: https://github.com/open-telemetry/opentelemetry-java/pull/6367/files#diff-d716139d47f651d6b6558f3e6f03aa59c98e52723f472b2c6a24bd02ec5fe2ceR340
• Ruby doesn't handle that at all.

Have you looked into ending the original span, and passing a wrapped version of that span to the OnEnding methods? That wrapped version would report that it is still recording and be the sole way to modify the wrapped span.

[dmathieu]

If the original span is ended, the wrapper couldn't propagate changes to it.

[MrAlias]

If the original span is ended, the wrapper couldn't propagate changes to it.

If you are doing so from within the sdk/trace package you should be able to. You have access to all the internal fields and methods. Why wouldn't that work?