Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement SpanProcessor::onEnding #1483

Draft
wants to merge 2 commits into
base: 2.x
Choose a base branch
from
+117 −4
Draft

implement SpanProcessor::onEnding #1483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5416b0
implement SpanProcessor::onEnding
brettmc Jan 23, 2025
972605b
mutable span in OnEnding, prevent async mutation
brettmc Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/SDK/Trace/Span.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ public function end(?int $endEpochNanos = null): void

$this->endEpochNanos = $endEpochNanos ?? Clock::getDefault()->now();
$this->hasEnded = true;
$this->spanProcessor->onEnding($this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$this->checkForDroppedElements();

Expand Down
4 changes: 4 additions & 0 deletions src/SDK/Trace/SpanProcessor/BatchSpanProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentCo
{
}

public function onEnding(ReadWriteSpanInterface $span): void
{
}

public function onEnd(ReadableSpanInterface $span): void
{
if ($this->closed) {
Expand Down
7 changes: 7 additions & 0 deletions src/SDK/Trace/SpanProcessor/MultiSpanProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,11 @@ public function forceFlush(?CancellationInterface $cancellation = null): bool

return $result;
}

public function onEnding(ReadWriteSpanInterface $span): void
{
foreach ($this->processors as $processor) {
$processor->onEnding($span);
}
}
}
4 changes: 4 additions & 0 deletions src/SDK/Trace/SpanProcessor/NoopSpanProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,8 @@ public function shutdown(?CancellationInterface $cancellation = null): bool
{
return $this->forceFlush();
}

public function onEnding(ReadWriteSpanInterface $span): void
{
} //@codeCoverageIgnore
}
4 changes: 4 additions & 0 deletions src/SDK/Trace/SpanProcessor/SimpleSpanProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,8 @@ private function flush(Closure $task, string $taskName, bool $propagateResult, C

return $success;
}

public function onEnding(ReadWriteSpanInterface $span): void
{
}
}
6 changes: 6 additions & 0 deletions src/SDK/Trace/SpanProcessorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ interface SpanProcessorInterface
*/
public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void;

/**
* @experimental
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.41.0/specification/trace/sdk.md#onending
*/
public function onEnding(ReadWriteSpanInterface $span): void;

/**
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#onendspan
*/
Expand Down
10 changes: 10 additions & 0 deletions tests/Unit/SDK/Trace/SpanTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,16 @@ public function test_end_span(): void
->once();
}

#[Group('trace-compliance')]
public function test_end_span_calls_processor_onending(): void
{
$span = $this->createTestSpan(API\SpanKind::KIND_CONSUMER);
$span->end();
$this->spanProcessor
->shouldHaveReceived('onEnding')
->once();
}

public function test_get_start_epoch_nanos(): void
{
$span = $this->createTestSpan(API\SpanKind::KIND_INTERNAL);
Expand Down
Loading