Implement span processor's OnEnding

[dmathieu]

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

Spec PR: https://github.com/open-telemetry/opentelemetry-specification/pull/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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.5%. Comparing base (be328ac) to head (f4ae0f4). Report is 420 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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

[dmathieu]

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

[dmathieu]

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

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

[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?

[dmathieu]

Closing as stale.