Skip to content

Commit

Permalink
Prevent recording of child spans of a 'leaf' span
Browse files Browse the repository at this point in the history
asgrim committed Aug 26, 2021

Unverified

This user has not yet uploaded their public signing key.
1 parent 192fcaa commit 3fcc622
Showing 6 changed files with 99 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file, in reverse

- [#236](https://github.com/scoutapp/scout-apm-php/pull/236) Replace `doctrine/cache` with `cache/array-adapter` in tests
- Note - this change only affects our internal tests, the `Agent` still depends on *any* PSR-16 compatible cache (including `doctrine/cache`)
- [#237](https://github.com/scoutapp/scout-apm-php/pull/237) Made 'leaf span' recording more efficient so child spans are not recorded at all

### Deprecated

14 changes: 14 additions & 0 deletions src/Events/Request/Request.php
Original file line number Diff line number Diff line change
@@ -49,6 +49,8 @@ class Request implements CommandWithChildren
private $requestUriOverride;
/** @var int */
private $spanCount = 0;
/** @var int */
private $leafNodeDepth = 0;
/**
* @psalm-var ConfigKey::URI_REPORTING_*
* @var string
@@ -270,6 +272,12 @@ public function startSpan(string $operation, ?float $overrideTimestamp = null, b
throw SpanLimitReached::forOperation($operation, self::MAX_COMPLETE_SPANS);
}

if ($this->currentCommand instanceof Span && $this->currentCommand->isLeaf()) {
$this->leafNodeDepth++;

return $this->currentCommand;
}

$this->spanCount++;

$span = new Span($this->currentCommand, $operation, $this->id, $overrideTimestamp, $leafSpan);
@@ -299,6 +307,12 @@ public function stopSpan(?float $overrideTimestamp = null): void
return;
}

if ($command->isLeaf() && $this->leafNodeDepth > 0) {
$this->leafNodeDepth--;

return;
}

$command->stop($overrideTimestamp);

$this->currentCommand = $command->parent();
9 changes: 5 additions & 4 deletions src/Events/Span/Span.php
Original file line number Diff line number Diff line change
@@ -85,6 +85,11 @@ public function parent(): CommandWithChildren
return $this->parent;
}

public function isLeaf(): bool
{
return $this->leafSpan;
}

/**
* Do not call this directly - use Request#stopSpan() or Agent#stopSpan() to correctly handle bookkeeping
*
@@ -186,10 +191,6 @@ public function jsonSerialize(): array
];

foreach ($this->children as $child) {
if ($this->leafSpan && $child instanceof Span) {
continue;
}

foreach ($child->jsonSerialize() as $value) {
$commands[] = $value;
}
29 changes: 29 additions & 0 deletions tests/Unit/Events/Request/RequestTest.php
Original file line number Diff line number Diff line change
@@ -11,9 +11,11 @@
use Scoutapm\Events\Request\Request;
use Scoutapm\Events\Request\RequestId;
use Scoutapm\Events\Span\Span;
use Scoutapm\UnitTests\TestHelper;

use function array_key_exists;
use function array_map;
use function assert;
use function json_decode;
use function json_encode;
use function next;
@@ -345,4 +347,31 @@ public function testRequestIsTaggedWithQueueTime(string $headerName, string $hea

unset($_SERVER[$headerName]);
}

public function testSpansAreNotRecordedBelowLeafSpans(): void
{
$request = $this->requestFromConfiguration();

$request->startSpan('Foo', null, true);
$request->startSpan('ShouldNotBeRecorded1');
$request->startSpan('ShouldNotBeRecorded2');
$request->startSpan('ShouldNotBeRecorded3');
$request->stopSpan();
$request->stopSpan();
$request->startSpan('ShouldNotBeRecorded4');
$request->stopSpan();
$request->stopSpan();
$request->startSpan('ShouldNotBeRecorded5');
$request->stopSpan();
$request->stopSpan();

self::assertSame(1, $request->collectedSpans());

$firstSpan = TestHelper::firstChildForCommand($request);
assert($firstSpan instanceof Span);
self::assertSame('Foo', $firstSpan->getName());

$children = TestHelper::childrenForCommand($firstSpan);
self::assertCount(0, $children);
}
}
6 changes: 6 additions & 0 deletions tests/Unit/Events/Span/SpanTest.php
Original file line number Diff line number Diff line change
@@ -33,6 +33,12 @@ public function testCanBeInitialized(): void
self::assertNull($span->getStopTime());
}

public function testThatASpanKnowsItIsALeafNode(): void
{
$span = new Span($this->mockParent, 'name', RequestId::new(), null, true);
self::assertTrue($span->isLeaf());
}

/** @throws Exception */
public function testCanBeStopped(): void
{
44 changes: 44 additions & 0 deletions tests/Unit/TestHelper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

declare(strict_types=1);

namespace Scoutapm\UnitTests;

use InvalidArgumentException;
use ReflectionException;
use ReflectionProperty;
use Scoutapm\Connector\Command;
use Scoutapm\Connector\CommandWithChildren;
use Webmozart\Assert\Assert;

use function count;
use function reset;

final class TestHelper
{
/**
* @return Command[]
*
* @throws InvalidArgumentException|ReflectionException
*/
public static function childrenForCommand(CommandWithChildren $commandWithChildren): array
{
$childrenProperty = new ReflectionProperty($commandWithChildren, 'children');
$childrenProperty->setAccessible(true);
$children = $childrenProperty->getValue($commandWithChildren);

Assert::isArray($children);
Assert::allIsInstanceOf($children, Command::class);

return $children;
}

public static function firstChildForCommand(CommandWithChildren $commandWithChildren): Command
{
$children = self::childrenForCommand($commandWithChildren);

Assert::greaterThanEq(count($children), 1);

return reset($children);
}
}

0 comments on commit 3fcc622

Please sign in to comment.