From 9ae437000aa4f3da01a57a41a580c3f0b8f764c8 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Mon, 22 Feb 2021 19:29:23 +0100 Subject: [PATCH] General improvements --- composer.json | 18 ++-- psalm.xml | 1 + src/Resources/config/schema/sentry-1.0.xsd | 1 + src/Resources/config/services.xml | 1 + src/Tracing/Twig/TwigTracingExtension.php | 60 ++++++++++--- tests/Stubs/Profile.phpstub | 3 +- .../Tracing/Twig/TwigTracingExtensionTest.php | 87 ++++++++++--------- 7 files changed, 109 insertions(+), 62 deletions(-) diff --git a/composer.json b/composer.json index 0f86ec9b..6d625810 100644 --- a/composer.json +++ b/composer.json @@ -23,13 +23,13 @@ "jean85/pretty-package-versions": "^1.5 || ^2.0", "php-http/discovery": "^1.11", "sentry/sdk": "^3.1", - "symfony/config": "^3.4.44||^4.4.11||^5.0.11", - "symfony/console": "^3.4.44||^4.4.11||^5.0.11", - "symfony/dependency-injection": "^3.4.44||^4.4.11||^5.0.11", - "symfony/event-dispatcher": "^3.4.44||^4.4.11||^5.0.11", - "symfony/http-kernel": "^3.4.44||^4.4.11||^5.0.11", + "symfony/config": "^3.4.44||^4.4.12||^5.0.11", + "symfony/console": "^3.4.44||^4.4.12||^5.0.11", + "symfony/dependency-injection": "^3.4.44||^4.4.12||^5.0.11", + "symfony/event-dispatcher": "^3.4.44||^4.4.12||^5.0.11", + "symfony/http-kernel": "^3.4.44||^4.4.12||^5.0.11", "symfony/psr-http-message-bridge": "^2.0", - "symfony/security-core": "^3.4.44||^4.4.11||^5.0.11" + "symfony/security-core": "^3.4.44||^4.4.12||^5.0.11" }, "require-dev": { "doctrine/dbal": "^2.10||^3.0", @@ -46,18 +46,18 @@ "symfony/browser-kit": "^3.4.44||^4.4.12||^5.0.11", "symfony/dom-crawler": "^3.4.44||^4.4.12||^5.0.11", "symfony/framework-bundle": "^3.4.44||^4.4.12||^5.0.11", - "symfony/messenger": "^4.4.11||^5.0.11", + "symfony/messenger": "^4.4.12||^5.0.11", "symfony/monolog-bundle": "^3.4", "symfony/phpunit-bridge": "^5.0", "symfony/polyfill-php80": "^1.22", "symfony/twig-bundle": "^3.4.44||^4.4.12||^5.0.11", - "symfony/yaml": "^3.4.44||^4.4.11||^5.0.11", + "symfony/yaml": "^3.4.44||^4.4.12||^5.0.11", "vimeo/psalm": "^4.3" }, "suggest": { "monolog/monolog": "Allow sending log messages to Sentry by using the included Monolog handler.", "doctrine/doctrine-bundle": "Allow distributed tracing of database queries using Sentry.", - "symfony/twig-bundle": "Allow distributed tracing of twig template rendering using Sentry." + "symfony/twig-bundle": "Allow distributed tracing of Twig template rendering using Sentry." }, "autoload": { "files": [ diff --git a/psalm.xml b/psalm.xml index 5aaa0ee1..2810674c 100644 --- a/psalm.xml +++ b/psalm.xml @@ -13,6 +13,7 @@ + diff --git a/src/Resources/config/schema/sentry-1.0.xsd b/src/Resources/config/schema/sentry-1.0.xsd index 250e092c..fb2718d9 100644 --- a/src/Resources/config/schema/sentry-1.0.xsd +++ b/src/Resources/config/schema/sentry-1.0.xsd @@ -95,6 +95,7 @@ + diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index 8582dc2b..5710faf3 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -75,6 +75,7 @@ + diff --git a/src/Tracing/Twig/TwigTracingExtension.php b/src/Tracing/Twig/TwigTracingExtension.php index 645b1d5e..dda1ce1c 100644 --- a/src/Tracing/Twig/TwigTracingExtension.php +++ b/src/Tracing/Twig/TwigTracingExtension.php @@ -5,6 +5,7 @@ namespace Sentry\SentryBundle\Tracing\Twig; use Sentry\State\HubInterface; +use Sentry\Tracing\Span; use Sentry\Tracing\SpanContext; use SplObjectStorage; use Twig\Extension\AbstractExtension; @@ -19,9 +20,9 @@ final class TwigTracingExtension extends AbstractExtension private $hub; /** - * @var SplObjectStorage + * @var SplObjectStorage The currently active spans */ - private $events; + private $spans; /** * @param HubInterface $hub The current hub @@ -29,36 +30,73 @@ final class TwigTracingExtension extends AbstractExtension public function __construct(HubInterface $hub) { $this->hub = $hub; - $this->events = new SplObjectStorage(); + $this->spans = new SplObjectStorage(); } + /** + * This method is called before the execution of a block, a macro or a + * template. + * + * @param Profile $profile The profiling data + */ public function enter(Profile $profile): void { $transaction = $this->hub->getTransaction(); - if (null === $transaction || !$profile->isTemplate()) { + if (null === $transaction) { return; } $spanContext = new SpanContext(); - $spanContext->setOp('twig.template'); - $spanContext->setDescription($profile->getName()); + $spanContext->setOp('view.render'); + $spanContext->setDescription($this->getSpanDescription($profile)); - $this->events[$profile] = $transaction->startChild($spanContext); + $this->spans[$profile] = $transaction->startChild($spanContext); } + /** + * This method is called when the execution of a block, a macro or a + * template is finished. + * + * @param Profile $profile The profiling data + */ public function leave(Profile $profile): void { - if (empty($this->events[$profile]) || !$profile->isTemplate()) { + if (!isset($this->spans[$profile])) { return; } - $this->events[$profile]->finish(); - unset($this->events[$profile]); + $this->spans[$profile]->finish(); + + unset($this->spans[$profile]); } + /** + * {@inheritdoc} + */ public function getNodeVisitors(): array { - return [new ProfilerNodeVisitor(static::class)]; + return [ + new ProfilerNodeVisitor(self::class), + ]; + } + + /** + * Gets a short description for the span. + * + * @param Profile $profile The profiling data + */ + private function getSpanDescription(Profile $profile): string + { + switch (true) { + case $profile->isRoot(): + return $profile->getName(); + + case $profile->isTemplate(): + return $profile->getTemplate(); + + default: + return sprintf('%s::%s(%s)', $profile->getTemplate(), $profile->getType(), $profile->getName()); + } } } diff --git a/tests/Stubs/Profile.phpstub b/tests/Stubs/Profile.phpstub index cdbe9eb9..4f198fd2 100644 --- a/tests/Stubs/Profile.phpstub +++ b/tests/Stubs/Profile.phpstub @@ -6,4 +6,5 @@ namespace Twig\Profiler; * @template-implements \IteratorAggregate */ final class Profile implements \IteratorAggregate, \Serializable -{} +{ +} diff --git a/tests/Tracing/Twig/TwigTracingExtensionTest.php b/tests/Tracing/Twig/TwigTracingExtensionTest.php index 6a3be3df..32d65bae 100644 --- a/tests/Tracing/Twig/TwigTracingExtensionTest.php +++ b/tests/Tracing/Twig/TwigTracingExtensionTest.php @@ -30,66 +30,64 @@ protected function setUp(): void $this->listener = new TwigTracingExtension($this->hub); } - public function testThatTwigEnterProfileIgnoresTracingWhenTransactionIsNotStarted(): void + /** + * @dataProvider enterDataProvider + */ + public function testEnter(Profile $profile, string $spanDescription): void { - $this->hub->expects($this->once()) - ->method('getTransaction') - ->willReturn(null); - - $this->listener->enter(new Profile('main', Profile::TEMPLATE)); - } + $transaction = new Transaction(new TransactionContext()); + $transaction->initSpanRecorder(); - public function testThatTwigEnterProfileIgnoresTracingWhenNotATemplate(): void - { $this->hub->expects($this->once()) ->method('getTransaction') - ->willReturn(new Transaction(new TransactionContext())); - - $this->listener->enter(new Profile('main', Profile::ROOT)); - } + ->willReturn($transaction); - public function testThatTwigLeaveProfileIgnoresTracingWhenTransactionIsNotStarted(): void - { - $this->hub->expects($this->once()) - ->method('getTransaction') - ->willReturn(null); + $this->listener->enter($profile); - $profile = new Profile('main', Profile::TEMPLATE); + $spans = $transaction->getSpanRecorder()->getSpans(); - $this->listener->enter($profile); - $this->listener->leave($profile); + $this->assertCount(2, $spans); + $this->assertNull($spans[1]->getEndTimestamp()); + $this->assertSame('view.render', $spans[1]->getOp()); + $this->assertSame($spanDescription, $spans[1]->getDescription()); } - public function testThatTwigLeaveProfileIgnoresTracingWhenNotATemplate(): void + /** + * @return \Generator + */ + public function enterDataProvider(): \Generator { - $this->hub->expects($this->once()) - ->method('getTransaction') - ->willReturn(new Transaction(new TransactionContext())); - - $profile = new Profile('main', Profile::ROOT); - - $this->listener->enter($profile); - $this->listener->leave($profile); + yield [ + new Profile('main.twig', Profile::ROOT), + 'main', + ]; + + yield [ + new Profile('main.twig', Profile::TEMPLATE), + 'main.twig', + ]; + + yield [ + new Profile('main.twig', Profile::BLOCK, 'content'), + 'main.twig::block(content)', + ]; + + yield [ + new Profile('main.twig', Profile::MACRO, 'input'), + 'main.twig::macro(input)', + ]; } - public function testThatTwigEnterProfileAttachesAChildSpanWhenTransactionStarted(): void + public function testEnterDoesNothingIfNoSpanIsSetOnHub(): void { - $transaction = new Transaction(new TransactionContext()); - $transaction->initSpanRecorder(); - $this->hub->expects($this->once()) ->method('getTransaction') - ->willReturn($transaction); + ->willReturn(null); $this->listener->enter(new Profile('main', Profile::TEMPLATE)); - - $spans = $transaction->getSpanRecorder()->getSpans(); - - $this->assertCount(2, $spans); - $this->assertNull($spans[1]->getEndTimestamp()); } - public function testThatTwigLeaveProfileFinishesTheChildSpanWhenChildSpanStarted(): void + public function testLeave(): void { $transaction = new Transaction(new TransactionContext()); $transaction->initSpanRecorder(); @@ -108,4 +106,11 @@ public function testThatTwigLeaveProfileFinishesTheChildSpanWhenChildSpanStarted $this->assertCount(2, $spans); $this->assertNotNull($spans[1]->getEndTimestamp()); } + + public function testLeaveDoesNothingIfSpanDoesNotExistsForProfile(): void + { + $this->expectNotToPerformAssertions(); + + $this->listener->leave(new Profile('main', Profile::TEMPLATE)); + } }