Skip to content

Commit 82d9593

Browse files
stayallivecleptric
stayallive
and
cleptric
authored
Test span sampled status before creating child spans (getsentry#1740)
Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>
1 parent 7ed2844 commit 82d9593

File tree

4 files changed

+166
-48
lines changed

4 files changed

+166
-48
lines changed

‎src/Tracing/GuzzleTracingMiddleware.php‎

Lines changed: 33 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,7 @@ public static function trace(?HubInterface $hub = null): \Closure
2828
return static function (RequestInterface $request, array $options) use ($hub, $handler) {
2929
$hub = $hub ?? SentrySdk::getCurrentHub();
3030
$client = $hub->getClient();
31-
$span = $hub->getSpan();
32-
33-
if ($span === null) {
34-
if (self::shouldAttachTracingHeaders($client, $request)) {
35-
$request = $request
36-
->withHeader('sentry-trace', getTraceparent())
37-
->withHeader('traceparent', getW3CTraceparent())
38-
->withHeader('baggage', getBaggage());
39-
}
40-
41-
return $handler($request, $options);
42-
}
31+
$parentSpan = $hub->getSpan();
4332

4433
$partialUri = Uri::fromParts([
4534
'scheme' => $request->getUri()->getScheme(),
@@ -60,24 +49,30 @@ public static function trace(?HubInterface $hub = null): \Closure
6049
$spanAndBreadcrumbData['http.fragment'] = $request->getUri()->getFragment();
6150
}
6251

63-
$spanContext = new SpanContext();
64-
$spanContext->setOp('http.client');
65-
$spanContext->setDescription($request->getMethod() . ' ' . $partialUri);
66-
$spanContext->setData($spanAndBreadcrumbData);
52+
$childSpan = null;
53+
54+
if ($parentSpan !== null && $parentSpan->getSampled()) {
55+
$spanContext = new SpanContext();
56+
$spanContext->setOp('http.client');
57+
$spanContext->setDescription($request->getMethod() . ' ' . $partialUri);
58+
$spanContext->setData($spanAndBreadcrumbData);
6759

68-
$childSpan = $span->startChild($spanContext);
60+
$childSpan = $parentSpan->startChild($spanContext);
61+
}
6962

7063
if (self::shouldAttachTracingHeaders($client, $request)) {
7164
$request = $request
72-
->withHeader('sentry-trace', $childSpan->toTraceparent())
73-
->withHeader('traceparent', $childSpan->toW3CTraceparent())
74-
->withHeader('baggage', $childSpan->toBaggage());
65+
->withHeader('sentry-trace', getTraceparent())
66+
->withHeader('traceparent', getW3CTraceparent())
67+
->withHeader('baggage', getBaggage());
7568
}
7669

7770
$handlerPromiseCallback = static function ($responseOrException) use ($hub, $spanAndBreadcrumbData, $childSpan, $partialUri) {
78-
// We finish the span (which means setting the span end timestamp) first to ensure the measured time
79-
// the span spans is as close to only the HTTP request time and do the data collection afterwards
80-
$childSpan->finish();
71+
if ($childSpan !== null) {
72+
// We finish the span (which means setting the span end timestamp) first to ensure the measured time
73+
// the span spans is as close to only the HTTP request time and do the data collection afterwards
74+
$childSpan->finish();
75+
}
8176

8277
$response = null;
8378

@@ -91,11 +86,15 @@ public static function trace(?HubInterface $hub = null): \Closure
9186
if ($response !== null) {
9287
$spanAndBreadcrumbData['http.response.body.size'] = $response->getBody()->getSize();
9388
$spanAndBreadcrumbData['http.response.status_code'] = $response->getStatusCode();
89+
}
9490

95-
$childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode()));
96-
$childSpan->setData($spanAndBreadcrumbData);
97-
} else {
98-
$childSpan->setStatus(SpanStatus::internalError());
91+
if ($childSpan !== null) {
92+
if ($response !== null) {
93+
$childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode()));
94+
$childSpan->setData($spanAndBreadcrumbData);
95+
} else {
96+
$childSpan->setStatus(SpanStatus::internalError());
97+
}
9998
}
10099

101100
$hub->addBreadcrumb(new Breadcrumb(
@@ -122,18 +121,14 @@ public static function trace(?HubInterface $hub = null): \Closure
122121

123122
private static function shouldAttachTracingHeaders(?ClientInterface $client, RequestInterface $request): bool
124123
{
125-
if ($client !== null) {
126-
$sdkOptions = $client->getOptions();
127-
128-
// Check if the request destination is allow listed in the trace_propagation_targets option.
129-
if (
130-
$sdkOptions->getTracePropagationTargets() === null
131-
|| \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets())
132-
) {
133-
return true;
134-
}
124+
if ($client === null) {
125+
return false;
135126
}
136127

137-
return false;
128+
$sdkOptions = $client->getOptions();
129+
130+
// Check if the request destination is allow listed in the trace_propagation_targets option.
131+
return $sdkOptions->getTracePropagationTargets() === null
132+
|| \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets());
138133
}
139134
}

‎src/functions.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -249,10 +249,10 @@ function trace(callable $trace, SpanContext $context)
249249
return SentrySdk::getCurrentHub()->withScope(function (Scope $scope) use ($context, $trace) {
250250
$parentSpan = $scope->getSpan();
251251

252-
// If there's a span set on the scope there is a transaction
253-
// active currently. If that is the case we create a child span
254-
// and set it on the scope. Otherwise we only execute the callable
255-
if ($parentSpan !== null) {
252+
// If there is a span set on the scope and it's sampled there is an active transaction.
253+
// If that is the case we create the child span and set it on the scope.
254+
// Otherwise we only execute the callable without creating a span.
255+
if ($parentSpan !== null && $parentSpan->getSampled()) {
256256
$span = $parentSpan->startChild($context);
257257

258258
$scope->setSpan($span);

‎tests/FunctionsTest.php‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,8 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void
364364
{
365365
$hub = new Hub();
366366

367-
$transaction = new Transaction(new TransactionContext());
367+
$transaction = new Transaction(TransactionContext::make());
368+
$transaction->setSampled(true);
368369

369370
$hub->setSpan($transaction);
370371

@@ -387,6 +388,30 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void
387388
}
388389
}
389390

391+
public function testTraceDoesntCreateSpanIfTransactionIsNotSampled(): void
392+
{
393+
$scope = $this->createMock(Scope::class);
394+
395+
$hub = new Hub(null, $scope);
396+
397+
$transaction = new Transaction(TransactionContext::make());
398+
$transaction->setSampled(false);
399+
400+
$scope->expects($this->never())
401+
->method('setSpan');
402+
$scope->expects($this->exactly(3))
403+
->method('getSpan')
404+
->willReturn($transaction);
405+
406+
SentrySdk::setCurrentHub($hub);
407+
408+
trace(function () use ($transaction, $hub) {
409+
$this->assertSame($transaction, $hub->getSpan());
410+
}, SpanContext::make());
411+
412+
$this->assertSame($transaction, $hub->getSpan());
413+
}
414+
390415
public function testTraceparentWithTracingDisabled(): void
391416
{
392417
$propagationContext = PropagationContext::fromDefaults();

‎tests/Tracing/GuzzleTracingMiddlewareTest.php‎

Lines changed: 103 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,21 @@
2323

2424
final class GuzzleTracingMiddlewareTest extends TestCase
2525
{
26-
public function testTraceDoesNothingIfSpanIsNotSet(): void
26+
public function testTraceCreatesBreadcrumbIfSpanIsNotSet(): void
2727
{
2828
$client = $this->createMock(ClientInterface::class);
29-
$client->expects($this->once())
29+
$client->expects($this->atLeast(2))
3030
->method('getOptions')
31-
->willReturn(new Options());
31+
->willReturn(new Options([
32+
'traces_sample_rate' => 0,
33+
]));
3234

3335
$hub = new Hub($client);
3436

37+
$transaction = $hub->startTransaction(TransactionContext::make());
38+
39+
$this->assertFalse($transaction->getSampled());
40+
3541
$expectedPromiseResult = new Response();
3642

3743
$middleware = GuzzleTracingMiddleware::trace($hub);
@@ -50,12 +56,59 @@ public function testTraceDoesNothingIfSpanIsNotSet(): void
5056

5157
$this->assertSame($expectedPromiseResult, $promiseResult);
5258

59+
$this->assertNull($transaction->getSpanRecorder());
60+
5361
$hub->configureScope(function (Scope $scope): void {
5462
$event = Event::createEvent();
5563

5664
$scope->applyToEvent($event);
5765

58-
$this->assertCount(0, $event->getBreadcrumbs());
66+
$this->assertCount(1, $event->getBreadcrumbs());
67+
});
68+
}
69+
70+
public function testTraceCreatesBreadcrumbIfSpanIsRecorded(): void
71+
{
72+
$client = $this->createMock(ClientInterface::class);
73+
$client->expects($this->atLeast(2))
74+
->method('getOptions')
75+
->willReturn(new Options([
76+
'traces_sample_rate' => 1,
77+
]));
78+
79+
$hub = new Hub($client);
80+
81+
$transaction = $hub->startTransaction(TransactionContext::make());
82+
83+
$this->assertTrue($transaction->getSampled());
84+
85+
$expectedPromiseResult = new Response();
86+
87+
$middleware = GuzzleTracingMiddleware::trace($hub);
88+
$function = $middleware(static function () use ($expectedPromiseResult): PromiseInterface {
89+
return new FulfilledPromise($expectedPromiseResult);
90+
});
91+
92+
/** @var PromiseInterface $promise */
93+
$promise = $function(new Request('GET', 'https://www.example.com'), []);
94+
95+
try {
96+
$promiseResult = $promise->wait();
97+
} catch (\Throwable $exception) {
98+
$promiseResult = $exception;
99+
}
100+
101+
$this->assertSame($expectedPromiseResult, $promiseResult);
102+
103+
$this->assertNotNull($transaction->getSpanRecorder());
104+
$this->assertCount(1, $transaction->getSpanRecorder()->getSpans());
105+
106+
$hub->configureScope(function (Scope $scope): void {
107+
$event = Event::createEvent();
108+
109+
$scope->applyToEvent($event);
110+
111+
$this->assertCount(1, $event->getBreadcrumbs());
59112
});
60113
}
61114

@@ -95,7 +148,7 @@ public function testTraceHeaders(Request $request, Options $options, bool $heade
95148
/**
96149
* @dataProvider traceHeadersDataProvider
97150
*/
98-
public function testTraceHeadersWithTransacttion(Request $request, Options $options, bool $headersShouldBePresent): void
151+
public function testTraceHeadersWithTransaction(Request $request, Options $options, bool $headersShouldBePresent): void
99152
{
100153
$client = $this->createMock(ClientInterface::class);
101154
$client->expects($this->atLeast(2))
@@ -133,6 +186,15 @@ public function testTraceHeadersWithTransacttion(Request $request, Options $opti
133186

134187
public static function traceHeadersDataProvider(): iterable
135188
{
189+
// Test cases here are duplicated with sampling enabled and disabled because trace headers hould be added regardless of the sample decision
190+
191+
yield [
192+
new Request('GET', 'https://www.example.com'),
193+
new Options([
194+
'traces_sample_rate' => 0,
195+
]),
196+
true,
197+
];
136198
yield [
137199
new Request('GET', 'https://www.example.com'),
138200
new Options([
@@ -141,6 +203,14 @@ public static function traceHeadersDataProvider(): iterable
141203
true,
142204
];
143205

206+
yield [
207+
new Request('GET', 'https://www.example.com'),
208+
new Options([
209+
'traces_sample_rate' => 0,
210+
'trace_propagation_targets' => null,
211+
]),
212+
true,
213+
];
144214
yield [
145215
new Request('GET', 'https://www.example.com'),
146216
new Options([
@@ -150,6 +220,16 @@ public static function traceHeadersDataProvider(): iterable
150220
true,
151221
];
152222

223+
yield [
224+
new Request('GET', 'https://www.example.com'),
225+
new Options([
226+
'traces_sample_rate' => 0,
227+
'trace_propagation_targets' => [
228+
'www.example.com',
229+
],
230+
]),
231+
true,
232+
];
153233
yield [
154234
new Request('GET', 'https://www.example.com'),
155235
new Options([
@@ -161,6 +241,14 @@ public static function traceHeadersDataProvider(): iterable
161241
true,
162242
];
163243

244+
yield [
245+
new Request('GET', 'https://www.example.com'),
246+
new Options([
247+
'traces_sample_rate' => 0,
248+
'trace_propagation_targets' => [],
249+
]),
250+
false,
251+
];
164252
yield [
165253
new Request('GET', 'https://www.example.com'),
166254
new Options([
@@ -170,6 +258,16 @@ public static function traceHeadersDataProvider(): iterable
170258
false,
171259
];
172260

261+
yield [
262+
new Request('GET', 'https://www.example.com'),
263+
new Options([
264+
'traces_sample_rate' => 0,
265+
'trace_propagation_targets' => [
266+
'example.com',
267+
],
268+
]),
269+
false,
270+
];
173271
yield [
174272
new Request('GET', 'https://www.example.com'),
175273
new Options([

0 commit comments

Comments
 (0)