implement SpanProcessor::onEnding

[brettmc]

added to spec 1.36.0

[codecov]

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.28%. Comparing base (27dfc42) to head (a5416b0).

Files with missing lines	Patch %	Lines
src/SDK/Trace/SpanProcessor/MultiSpanProcessor.php	0.00%	3 Missing ⚠️
src/SDK/Trace/SpanProcessor/BatchSpanProcessor.php	0.00%	2 Missing ⚠️
...rc/SDK/Trace/SpanProcessor/SimpleSpanProcessor.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #1483      +/-   ##
============================================
- Coverage     72.37%   72.28%   -0.09%     
- Complexity     2733     2737       +4     
============================================
  Files           402      402              
  Lines          8169     8177       +8     
============================================
- Hits           5912     5911       -1     
- Misses         2257     2266       +9     

Flag	Coverage Δ
8.2	72.19% <12.50%> (-0.06%)	⬇️
8.3	72.19% <12.50%> (-0.08%)	⬇️
8.4	72.14% <12.50%> (-0.06%)	⬇️
8.5	72.11% <12.50%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/SDK/Trace/Span.php	94.66% <100.00%> (+0.03%)	⬆️
src/SDK/Trace/SpanProcessor/NoopSpanProcessor.php	87.50% <ø> (ø)
src/SDK/Trace/SpanProcessor/BatchSpanProcessor.php	98.51% <0.00%> (-1.49%)	⬇️
...rc/SDK/Trace/SpanProcessor/SimpleSpanProcessor.php	95.55% <0.00%> (-4.45%)	⬇️
src/SDK/Trace/SpanProcessor/MultiSpanProcessor.php	88.46% <0.00%> (-11.54%)	⬇️

... and 2 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27dfc42...a5416b0. Read the comment docs.

[Nevay]

The Span object MUST still be mutable while OnEnding is called.

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.

[brettmc]

Which part are you querying? $this is a Span implementing ReadWriteSpanInterface so is still mutable (and there's a test to show this working). I don't think we need to worry about other threads, and I don't see any other opportunity for modifying the span...

[Nevay]

$this is a Span implementing ReadWriteSpanInterface so is still mutable

Setting $this->hasEnded = true prevents all further writes to the span (besides ::setAttributes(), which is a bug); currently ::onEnding() cannot modify the span, the following example fails:

$tracerProvider = (new TracerProviderBuilder())
    ->addSpanProcessor(new class implements SpanProcessorInterface {

        public function onEnding(ReadWriteSpanInterface $span): void {
            $span->updateName('updated');
            assert($span->getName() === 'updated');
        }

        public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void {}
        public function onEnd(ReadableSpanInterface $span): void {}
        public function forceFlush(?CancellationInterface $cancellation = null): bool { return true; }
        public function shutdown(?CancellationInterface $cancellation = null): bool { return true; }
    })
    ->build();

$span = $tracerProvider->getTracer('test')->spanBuilder('span')->startSpan();
$span->end();

I don't think we need to worry about other threads, and I don't see any other opportunity for modifying the span...

Most mentions of "other threads" also apply to fibers/coroutines, for example the following shouldn't throw an assertion error after fixing the issue above:

$tracerProvider = (new TracerProviderBuilder())
    ->addSpanProcessor(new class implements SpanProcessorInterface {

        public function onEnding(ReadWriteSpanInterface $span): void {
            $spanName = $span->getName();

            delay(0);
            assert($span->getName() === $spanName);
        }

        public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void {}
        public function onEnd(ReadableSpanInterface $span): void {}
        public function forceFlush(?CancellationInterface $cancellation = null): bool { return true; }
        public function shutdown(?CancellationInterface $cancellation = null): bool { return true; }
    })
    ->build();

$span = $tracerProvider->getTracer('test')->spanBuilder('span')->startSpan();
async($span->updateName(...), 'should-not-update');
$span->end();

---

One solution would be to pass a new span object to ::onEnding() while still hasEnded=true ending the original span.
A basic implementation (which will create unnecessary array copies on events/links modification that could be avoided by storing the state in a separate object):

public function end(?int $endEpochNanos = null): void
{
    if ($this->endEpochNanos) { // should be nullable and check for !== null instead
        return;
    }

    $this->endEpochNanos = $endEpochNanos ?? Clock::getDefault()->now();
    $span = clone $this;
    $this->hasEnded = true;
    $this->spanProcessor->onEnding($span);
    $span->hasEnded = true;

    $span->checkForDroppedElements();

    $this->spanProcessor->onEnd($span);
}

[brettmc]

Sorry, my bad. I am confusing this PR with #1482 which is similar but for logs. Back to draft while I work on this more.