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

Test span sampled status before creating child spans #1740

Merged
merged 4 commits into from
May 27, 2024
Merged

Test span sampled status before creating child spans #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfec37e
Also test the sampled state of a span before creating a child
stayallive May 16, 2024
b501bef
Fix Guzzle tracing middleware logic
stayallive May 16, 2024
b79a3cd
Add more test cases
stayallive May 24, 2024
f54b8a2
Merge branch 'master' into span-sampled-checks
cleptric May 27, 2024
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
71 changes: 33 additions & 38 deletions src/Tracing/GuzzleTracingMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,7 @@ public static function trace(?HubInterface $hub = null): \Closure
return static function (RequestInterface $request, array $options) use ($hub, $handler) {
$hub = $hub ?? SentrySdk::getCurrentHub();
$client = $hub->getClient();
$span = $hub->getSpan();

if ($span === null) {
if (self::shouldAttachTracingHeaders($client, $request)) {
$request = $request
->withHeader('sentry-trace', getTraceparent())
->withHeader('traceparent', getW3CTraceparent())
->withHeader('baggage', getBaggage());
}

return $handler($request, $options);
}
$parentSpan = $hub->getSpan();

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

$spanContext = new SpanContext();
$spanContext->setOp('http.client');
$spanContext->setDescription($request->getMethod() . ' ' . $partialUri);
$spanContext->setData($spanAndBreadcrumbData);
$childSpan = null;

if ($parentSpan !== null && $parentSpan->getSampled()) {
$spanContext = new SpanContext();
$spanContext->setOp('http.client');
$spanContext->setDescription($request->getMethod() . ' ' . $partialUri);
$spanContext->setData($spanAndBreadcrumbData);

$childSpan = $span->startChild($spanContext);
$childSpan = $parentSpan->startChild($spanContext);
}

if (self::shouldAttachTracingHeaders($client, $request)) {
$request = $request
->withHeader('sentry-trace', $childSpan->toTraceparent())
->withHeader('traceparent', $childSpan->toW3CTraceparent())
->withHeader('baggage', $childSpan->toBaggage());
->withHeader('sentry-trace', getTraceparent())
->withHeader('traceparent', getW3CTraceparent())
->withHeader('baggage', getBaggage());
}

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

$response = null;

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

$childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode()));
$childSpan->setData($spanAndBreadcrumbData);
} else {
$childSpan->setStatus(SpanStatus::internalError());
if ($childSpan !== null) {
if ($response !== null) {
$childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode()));
$childSpan->setData($spanAndBreadcrumbData);
} else {
$childSpan->setStatus(SpanStatus::internalError());
}
}

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

private static function shouldAttachTracingHeaders(?ClientInterface $client, RequestInterface $request): bool
{
if ($client !== null) {
$sdkOptions = $client->getOptions();

// Check if the request destination is allow listed in the trace_propagation_targets option.
if (
$sdkOptions->getTracePropagationTargets() === null
|| \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets())
) {
return true;
}
if ($client === null) {
return false;
}

return false;
$sdkOptions = $client->getOptions();

// Check if the request destination is allow listed in the trace_propagation_targets option.
return $sdkOptions->getTracePropagationTargets() === null
|| \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets());
}
}
8 changes: 4 additions & 4 deletions src/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,10 @@ function trace(callable $trace, SpanContext $context)
return SentrySdk::getCurrentHub()->withScope(function (Scope $scope) use ($context, $trace) {
$parentSpan = $scope->getSpan();

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

$scope->setSpan($span);
Expand Down
27 changes: 26 additions & 1 deletion tests/FunctionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void
{
$hub = new Hub();

$transaction = new Transaction(new TransactionContext());
$transaction = new Transaction(TransactionContext::make());
$transaction->setSampled(true);

$hub->setSpan($transaction);

Expand All @@ -387,6 +388,30 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void
}
}

public function testTraceDoesntCreateSpanIfTransactionIsNotSampled(): void
{
$scope = $this->createMock(Scope::class);

$hub = new Hub(null, $scope);

$transaction = new Transaction(TransactionContext::make());
$transaction->setSampled(false);

$scope->expects($this->never())
->method('setSpan');
$scope->expects($this->exactly(3))
->method('getSpan')
->willReturn($transaction);

SentrySdk::setCurrentHub($hub);

trace(function () use ($transaction, $hub) {
$this->assertSame($transaction, $hub->getSpan());
}, SpanContext::make());

$this->assertSame($transaction, $hub->getSpan());
}

public function testTraceparentWithTracingDisabled(): void
{
$propagationContext = PropagationContext::fromDefaults();
Expand Down
108 changes: 103 additions & 5 deletions tests/Tracing/GuzzleTracingMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,21 @@

final class GuzzleTracingMiddlewareTest extends TestCase
{
public function testTraceDoesNothingIfSpanIsNotSet(): void
public function testTraceCreatesBreadcrumbIfSpanIsNotSet(): void
{
$client = $this->createMock(ClientInterface::class);
$client->expects($this->once())
$client->expects($this->atLeast(2))
->method('getOptions')
->willReturn(new Options());
->willReturn(new Options([
'traces_sample_rate' => 0,
]));

$hub = new Hub($client);

$transaction = $hub->startTransaction(TransactionContext::make());

$this->assertFalse($transaction->getSampled());

$expectedPromiseResult = new Response();

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

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

$this->assertNull($transaction->getSpanRecorder());

$hub->configureScope(function (Scope $scope): void {
$event = Event::createEvent();

$scope->applyToEvent($event);

$this->assertCount(0, $event->getBreadcrumbs());
$this->assertCount(1, $event->getBreadcrumbs());
});
}

public function testTraceCreatesBreadcrumbIfSpanIsRecorded(): void
{
$client = $this->createMock(ClientInterface::class);
$client->expects($this->atLeast(2))
->method('getOptions')
->willReturn(new Options([
'traces_sample_rate' => 1,
]));

$hub = new Hub($client);

$transaction = $hub->startTransaction(TransactionContext::make());

$this->assertTrue($transaction->getSampled());

$expectedPromiseResult = new Response();

$middleware = GuzzleTracingMiddleware::trace($hub);
$function = $middleware(static function () use ($expectedPromiseResult): PromiseInterface {
return new FulfilledPromise($expectedPromiseResult);
});

/** @var PromiseInterface $promise */
$promise = $function(new Request('GET', 'https://www.example.com'), []);

try {
$promiseResult = $promise->wait();
} catch (\Throwable $exception) {
$promiseResult = $exception;
}

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

$this->assertNotNull($transaction->getSpanRecorder());
$this->assertCount(1, $transaction->getSpanRecorder()->getSpans());

$hub->configureScope(function (Scope $scope): void {
$event = Event::createEvent();

$scope->applyToEvent($event);

$this->assertCount(1, $event->getBreadcrumbs());
});
}

Expand Down Expand Up @@ -95,7 +148,7 @@ public function testTraceHeaders(Request $request, Options $options, bool $heade
/**
* @dataProvider traceHeadersDataProvider
*/
public function testTraceHeadersWithTransacttion(Request $request, Options $options, bool $headersShouldBePresent): void
public function testTraceHeadersWithTransaction(Request $request, Options $options, bool $headersShouldBePresent): void
{
$client = $this->createMock(ClientInterface::class);
$client->expects($this->atLeast(2))
Expand Down Expand Up @@ -133,6 +186,15 @@ public function testTraceHeadersWithTransacttion(Request $request, Options $opti

public static function traceHeadersDataProvider(): iterable
{
// Test cases here are duplicated with sampling enabled and disabled because trace headers hould be added regardless of the sample decision

yield [
new Request('GET', 'https://www.example.com'),
new Options([
'traces_sample_rate' => 0,
]),
true,
];
yield [
new Request('GET', 'https://www.example.com'),
new Options([
Expand All @@ -141,6 +203,14 @@ public static function traceHeadersDataProvider(): iterable
true,
];

yield [
new Request('GET', 'https://www.example.com'),
new Options([
'traces_sample_rate' => 0,
'trace_propagation_targets' => null,
]),
true,
];
yield [
new Request('GET', 'https://www.example.com'),
new Options([
Expand All @@ -150,6 +220,16 @@ public static function traceHeadersDataProvider(): iterable
true,
];

yield [
new Request('GET', 'https://www.example.com'),
new Options([
'traces_sample_rate' => 0,
'trace_propagation_targets' => [
'www.example.com',
],
]),
true,
];
yield [
new Request('GET', 'https://www.example.com'),
new Options([
Expand All @@ -161,6 +241,14 @@ public static function traceHeadersDataProvider(): iterable
true,
];

yield [
new Request('GET', 'https://www.example.com'),
new Options([
'traces_sample_rate' => 0,
'trace_propagation_targets' => [],
]),
false,
];
yield [
new Request('GET', 'https://www.example.com'),
new Options([
Expand All @@ -170,6 +258,16 @@ public static function traceHeadersDataProvider(): iterable
false,
];

yield [
new Request('GET', 'https://www.example.com'),
new Options([
'traces_sample_rate' => 0,
'trace_propagation_targets' => [
'example.com',
],
]),
false,
];
yield [
new Request('GET', 'https://www.example.com'),
new Options([
Expand Down
Loading