Skip to content

Commit 86fb69d

Browse files
brettmcbrettmc
authored
implement SpanProcessor::onEnding (open-telemetry#1483)
* added to spec 1.36.0 * mutable span in OnEnding, prevent async mutation (a clone rather than the original span is passed to onEnd) * change composer/tools cache key cached vendor dir was causing havoc with a conflict between vendor and vendor-bin/psalm (amphp/amp)
1 parent 3fa6101 commit 86fb69d

File tree

14 files changed

+225
-17
lines changed

14 files changed

+225
-17
lines changed

‎.github/workflows/php.yml‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,21 +84,17 @@ jobs:
8484
uses: actions/cache@v4
8585
with:
8686
path: vendor
87-
key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
88-
restore-keys: |
89-
${{ runner.os }}-${{ matrix.php-version }}-php-
87+
key: ${{ runner.os }}-${{ matrix.php-version }}-composer-${{ hashFiles('**/composer.json') }}
9088
- name: Cache test tools
9189
id: test-tools-cache
9290
uses: actions/cache@v4
9391
with:
9492
path: vendor-bin
95-
key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
96-
restore-keys: |
97-
${{ runner.os }}-${{ matrix.php-version }}-php-
93+
key: ${{ runner.os }}-${{ matrix.php-version }}-tools-${{ hashFiles('**/composer.json') }}
9894

9995
- name: Install dependencies
10096
id: composer
101-
if: steps.composer-cache.outputs.cache-hit != 'true'
97+
if: steps.composer-cache.outputs.cache-hit != 'true' || steps.test-tools-cache.outputs.cache-hit != 'true'
10298
run: composer install --prefer-dist --no-progress ${{ matrix.composer_args }}
10399

104100
- name: Check Style

‎composer.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
"require-dev": {
8888
"ext-grpc": "*",
8989
"grpc/grpc": "^1.30",
90+
"amphp/amp": "^3",
9091
"bamarni/composer-bin-plugin": "^1.8",
9192
"dg/bypass-finals": "^1.4",
9293
"guzzlehttp/guzzle": "^7.4",

‎src/SDK/Trace/Span.php‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ final class Span extends API\Span implements ReadWriteSpanInterface
2424
private array $events = [];
2525
private int $totalRecordedEvents = 0;
2626
private StatusDataInterface $status;
27-
private int $endEpochNanos = 0;
27+
private ?int $endEpochNanos = null;
2828
private bool $hasEnded = false;
2929

3030
/**
@@ -138,6 +138,10 @@ public function setAttribute(string $key, $value): self
138138
/** @inheritDoc */
139139
public function setAttributes(iterable $attributes): self
140140
{
141+
if ($this->hasEnded) {
142+
return $this;
143+
}
144+
141145
foreach ($attributes as $key => $value) {
142146
$this->attributesBuilder[$key] = $value;
143147
}
@@ -244,19 +248,24 @@ public function setStatus(string $code, ?string $description = null): self
244248
return $this;
245249
}
246250

247-
/** @inheritDoc */
251+
/**
252+
* @inheritDoc
253+
* @psalm-suppress NoInterfaceProperties,UndefinedInterfaceMethod
254+
**/
248255
public function end(?int $endEpochNanos = null): void
249256
{
250-
if ($this->hasEnded) {
257+
if ($this->endEpochNanos !== null) {
251258
return;
252259
}
253260

254261
$this->endEpochNanos = $endEpochNanos ?? Clock::getDefault()->now();
255-
$this->hasEnded = true;
256-
257-
$this->checkForDroppedElements();
262+
$span = clone $this;
263+
$this->hasEnded = true; // prevent further modifications to the span by async code
264+
$this->spanProcessor->onEnding($span);
265+
$span->hasEnded = true;
258266

259-
$this->spanProcessor->onEnd($this);
267+
$this->spanProcessor->onEnd($span);
268+
$span->checkForDroppedElements();
260269
}
261270

262271
/** @inheritDoc */
@@ -291,7 +300,7 @@ public function toSpanData(): SpanDataInterface
291300
$this->totalRecordedLinks,
292301
$this->totalRecordedEvents,
293302
$this->status,
294-
$this->endEpochNanos,
303+
$this->endEpochNanos ?? 0,
295304
$this->hasEnded
296305
);
297306
}

‎src/SDK/Trace/SpanProcessor/BatchSpanProcessor.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,10 @@ public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentCo
143143
{
144144
}
145145

146+
public function onEnding(ReadWriteSpanInterface $span): void
147+
{
148+
}
149+
146150
public function onEnd(ReadableSpanInterface $span): void
147151
{
148152
if ($this->closed) {

‎src/SDK/Trace/SpanProcessor/MultiSpanProcessor.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,11 @@ public function forceFlush(?CancellationInterface $cancellation = null): bool
7676

7777
return $result;
7878
}
79+
80+
public function onEnding(ReadWriteSpanInterface $span): void
81+
{
82+
foreach ($this->processors as $processor) {
83+
$processor->onEnding($span);
84+
}
85+
}
7986
}

‎src/SDK/Trace/SpanProcessor/NoopSpanProcessor.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,8 @@ public function shutdown(?CancellationInterface $cancellation = null): bool
4444
{
4545
return $this->forceFlush();
4646
}
47+
48+
public function onEnding(ReadWriteSpanInterface $span): void
49+
{
50+
} //@codeCoverageIgnore
4751
}

‎src/SDK/Trace/SpanProcessor/SimpleSpanProcessor.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,8 @@ private function flush(Closure $task, string $taskName, bool $propagateResult, C
113113

114114
return $success;
115115
}
116+
117+
public function onEnding(ReadWriteSpanInterface $span): void
118+
{
119+
}
116120
}

‎src/SDK/Trace/SpanProcessorInterface.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ interface SpanProcessorInterface
1515
*/
1616
public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void;
1717

18+
/**
19+
* @experimental
20+
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.41.0/specification/trace/sdk.md#onending
21+
*/
22+
public function onEnding(ReadWriteSpanInterface $span): void;
23+
1824
/**
1925
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#onendspan
2026
*/

‎tests/Integration/SDK/Trace/test_async_cannot_modify_span_during_onending.phpt‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
--TEST--
2+
Test that async code cannot update span during OnEnding
3+
--DESCRIPTION--
4+
The SDK MUST guarantee that the span can no longer be modified by any other thread before invoking OnEnding of the first SpanProcessor.
5+
--FILE--
6+
<?php
7+
require_once 'vendor/autoload.php';
8+
9+
use OpenTelemetry\SDK\Trace\TracerProviderBuilder;
10+
use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
11+
use OpenTelemetry\SDK\Trace\ReadableSpanInterface;
12+
use OpenTelemetry\SDK\Common\Future\CancellationInterface;
13+
use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
14+
use OpenTelemetry\Context\ContextInterface;
15+
16+
$tracerProvider = (new TracerProviderBuilder())
17+
->addSpanProcessor(new class implements SpanProcessorInterface {
18+
19+
public function onEnding(ReadWriteSpanInterface $span): void {
20+
$spanName = $span->getName();
21+
22+
Amp\delay(0);
23+
assert($span->getName() === $spanName);
24+
}
25+
26+
public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void {}
27+
public function onEnd(ReadableSpanInterface $span): void {}
28+
public function forceFlush(?CancellationInterface $cancellation = null): bool { return true; }
29+
public function shutdown(?CancellationInterface $cancellation = null): bool { return true; }
30+
})
31+
->build();
32+
33+
$span = $tracerProvider->getTracer('test')->spanBuilder('span')->startSpan();
34+
Amp\async($span->updateName(...), 'should-not-update');
35+
$span->end();
36+
?>
37+
--EXPECT--

‎tests/Integration/SDK/Trace/test_end_span_in_onending.phpt‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
--TEST--
2+
Test ending a span during OnEnding
3+
--FILE--
4+
<?php
5+
require_once 'vendor/autoload.php';
6+
7+
use OpenTelemetry\SDK\Trace\TracerProviderBuilder;
8+
use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
9+
use OpenTelemetry\SDK\Trace\ReadableSpanInterface;
10+
use OpenTelemetry\SDK\Common\Future\CancellationInterface;
11+
use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
12+
use OpenTelemetry\Context\ContextInterface;
13+
14+
$tracerProvider = (new TracerProviderBuilder())
15+
->addSpanProcessor(new class implements SpanProcessorInterface {
16+
17+
public function onEnding(ReadWriteSpanInterface $span): void {
18+
$span->end();
19+
}
20+
21+
public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void {}
22+
public function onEnd(ReadableSpanInterface $span): void {}
23+
public function forceFlush(?CancellationInterface $cancellation = null): bool { return true; }
24+
public function shutdown(?CancellationInterface $cancellation = null): bool { return true; }
25+
})
26+
->build();
27+
28+
$span = $tracerProvider->getTracer('test')->spanBuilder('span')->startSpan();
29+
$span->end();
30+
?>
31+
--EXPECT--

0 commit comments

Comments
 (0)