From cf0db4f31c1244e3161a7212d693bca2eb77fa82 Mon Sep 17 00:00:00 2001 From: Summer Date: Mon, 20 Nov 2023 19:27:17 +0800 Subject: [PATCH 1/4] Adding bindings to SQL spans in tracing --- config/sentry.php | 3 ++ src/Sentry/Laravel/Tracing/EventHandler.php | 14 +++++++ test/Sentry/TestCase.php | 13 +++++++ test/Sentry/Tracing/EventHandlerTest.php | 43 +++++++++++++++++++++ 4 files changed, 73 insertions(+) diff --git a/config/sentry.php b/config/sentry.php index 6476e31b..4fe68262 100644 --- a/config/sentry.php +++ b/config/sentry.php @@ -67,6 +67,9 @@ // Capture SQL queries as spans 'sql_queries' => env('SENTRY_TRACE_SQL_QUERIES_ENABLED', true), + // Capture SQL query bindings (parameters) in SQL query spans + 'sql_bindings' => env('SENTRY_TRACE_SQL_BINDINGS_ENABLED', false), + // Capture where the SQL query originated from on the SQL query spans 'sql_origin' => env('SENTRY_TRACE_SQL_ORIGIN_ENABLED', true), diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index b184ca82..a8bf89c3 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -41,6 +41,13 @@ class EventHandler */ private $traceSqlQueries; + /** + * Indicates if we should add query bindings to query spans. + * + * @var bool + */ + private $recordSqlBindings; + /** * Indicates if we should we add SQL query origin data to query spans. * @@ -83,6 +90,7 @@ public function __construct(array $config) { $this->traceSqlQueries = ($config['sql_queries'] ?? true) === true; $this->traceSqlQueryOrigins = ($config['sql_origin'] ?? true) === true; + $this->recordSqlBindings = ($config['sql_bindings'] ?? true) === true; $this->traceQueueJobs = ($config['queue_jobs'] ?? false) === true; $this->traceQueueJobsAsTransactions = ($config['queue_job_transactions'] ?? false) === true; @@ -176,6 +184,12 @@ protected function queryExecutedHandler(DatabaseEvents\QueryExecuted $query): vo } } + if ($this->recordSqlBindings) { + $context->setData(array_merge($context->getData(), [ + 'db.sql.bindings' => $query->bindings + ])); + } + $parentSpan->startChild($context); } diff --git a/test/Sentry/TestCase.php b/test/Sentry/TestCase.php index b6766421..fe23e48f 100644 --- a/test/Sentry/TestCase.php +++ b/test/Sentry/TestCase.php @@ -2,6 +2,8 @@ namespace Sentry\Laravel\Tests; +use Sentry\SentrySdk; +use Sentry\Tracing\Span; use Sentry\Tracing\Transaction; use Illuminate\Config\Repository; use Sentry\Tracing\TransactionContext; @@ -19,6 +21,7 @@ use Sentry\State\HubInterface; use Sentry\Laravel\ServiceProvider; use Orchestra\Testbench\TestCase as LaravelTestCase; +use Throwable; abstract class TestCase extends LaravelTestCase { @@ -155,6 +158,16 @@ protected function getLastSentryEvent(): ?Event return end(self::$lastSentryEvents)[0]; } + protected function getLastSentrySpan(): ?Span + { + try { + $spans = SentrySdk::getCurrentHub()->getSpan()->getSpanRecorder()->getSpans(); + return end($spans); + } catch (Throwable $throwable) { + return null; + } + } + protected function getLastEventSentryHint(): ?EventHint { if (empty(self::$lastSentryEvents)) { diff --git a/test/Sentry/Tracing/EventHandlerTest.php b/test/Sentry/Tracing/EventHandlerTest.php index 9c41e340..fed7883f 100644 --- a/test/Sentry/Tracing/EventHandlerTest.php +++ b/test/Sentry/Tracing/EventHandlerTest.php @@ -2,11 +2,18 @@ namespace Sentry\Laravel\Tests\Tracing; +use Illuminate\Database\Connection; +use Illuminate\Database\Events\QueryExecuted; +use Illuminate\Http\Request; +use Illuminate\Http\Response; +use Mockery; use ReflectionClass; use Sentry\Laravel\Tests\TestCase; use Sentry\Laravel\Tracing\BacktraceHelper; use RuntimeException; use Sentry\Laravel\Tracing\EventHandler; +use Sentry\Laravel\Tracing\Middleware; +use Sentry\SentrySdk; class EventHandlerTest extends TestCase { @@ -27,6 +34,31 @@ public function testAllMappedEventHandlersExist(): void ); } + public function testSqlBindingsAreRecordedWhenEnabled(): void + { + $this->resetApplicationWithConfig([ + 'sentry.traces_sample_rate' => 1, + 'sentry.tracing.sql_queries' => true, + 'sentry.tracing.sql_bindings' => true, + ]); + + $this->assertTrue($this->app['config']->get('sentry.tracing.sql_bindings')); + + $this->startTransaction(); + + $this->dispatchLaravelEvent(new QueryExecuted( + $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', + $bindings = ['1'], + 10, + $this->getMockedConnection() + )); + + $span = $this->getLastSentrySpan(); + + $this->assertEquals($query, $span->getDescription()); + $this->assertEquals($bindings, $span->getData()['db.sql.bindings']); + } + private function tryAllEventHandlerMethods(array $methods): void { $handler = new EventHandler([]); @@ -48,4 +80,15 @@ private function getEventHandlerMapFromEventHandler() return $attributes['eventHandlerMap']; } + + private function getMockedConnection() + { + $mock = Mockery::mock(Connection::class); + $mock->shouldReceive('getName')->andReturn('test'); + $mock->shouldReceive('getDatabaseName')->andReturn('test'); + $mock->shouldReceive('getDriverName')->andReturn('mysql'); + $mock->shouldReceive('getConfig')->andReturn(['host' => '127.0.0.1', 'port' => 3306]); + + return $mock; + } } From ffa1e75bf49a3353fc5f1c4799a8f85e420fb18b Mon Sep 17 00:00:00 2001 From: Summer Date: Tue, 21 Nov 2023 09:49:35 +0800 Subject: [PATCH 2/4] Add test cases --- test/Sentry/Tracing/EventHandlerTest.php | 34 +++++++++++++++++++----- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/test/Sentry/Tracing/EventHandlerTest.php b/test/Sentry/Tracing/EventHandlerTest.php index fed7883f..49209867 100644 --- a/test/Sentry/Tracing/EventHandlerTest.php +++ b/test/Sentry/Tracing/EventHandlerTest.php @@ -4,16 +4,11 @@ use Illuminate\Database\Connection; use Illuminate\Database\Events\QueryExecuted; -use Illuminate\Http\Request; -use Illuminate\Http\Response; use Mockery; use ReflectionClass; -use Sentry\Laravel\Tests\TestCase; -use Sentry\Laravel\Tracing\BacktraceHelper; use RuntimeException; +use Sentry\Laravel\Tests\TestCase; use Sentry\Laravel\Tracing\EventHandler; -use Sentry\Laravel\Tracing\Middleware; -use Sentry\SentrySdk; class EventHandlerTest extends TestCase { @@ -47,7 +42,7 @@ public function testSqlBindingsAreRecordedWhenEnabled(): void $this->startTransaction(); $this->dispatchLaravelEvent(new QueryExecuted( - $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', + $query = 'SELECT * FROM spans WHERE bindings = ?;', $bindings = ['1'], 10, $this->getMockedConnection() @@ -59,6 +54,31 @@ public function testSqlBindingsAreRecordedWhenEnabled(): void $this->assertEquals($bindings, $span->getData()['db.sql.bindings']); } + public function testSqlBindingsAreRecordedWhenDisabled(): void + { + $this->resetApplicationWithConfig([ + 'sentry.traces_sample_rate' => 1, + 'sentry.tracing.sql_queries' => true, + 'sentry.tracing.sql_bindings' => false, + ]); + + $this->assertFalse($this->app['config']->get('sentry.tracing.sql_bindings')); + + $this->startTransaction(); + + $this->dispatchLaravelEvent(new QueryExecuted( + $query = 'SELECT * FROM spans WHERE bindings = ?;', + $bindings = ['1'], + 10, + $this->getMockedConnection() + )); + + $span = $this->getLastSentrySpan(); + + $this->assertEquals($query, $span->getDescription()); + $this->assertFalse(isset($span->getData()['db.sql.bindings'])); + } + private function tryAllEventHandlerMethods(array $methods): void { $handler = new EventHandler([]); From 8e9e9b518b7c97c4d00b0d7a266b7f61a054ca19 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Wed, 22 Nov 2023 09:39:12 +0100 Subject: [PATCH 3/4] CS --- src/Sentry/Laravel/Tracing/EventHandler.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index a8bf89c3..aeb47814 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -46,7 +46,7 @@ class EventHandler * * @var bool */ - private $recordSqlBindings; + private $traceSqlBindings; /** * Indicates if we should we add SQL query origin data to query spans. @@ -89,8 +89,8 @@ class EventHandler public function __construct(array $config) { $this->traceSqlQueries = ($config['sql_queries'] ?? true) === true; + $this->traceSqlBindings = ($config['sql_bindings'] ?? true) === true; $this->traceSqlQueryOrigins = ($config['sql_origin'] ?? true) === true; - $this->recordSqlBindings = ($config['sql_bindings'] ?? true) === true; $this->traceQueueJobs = ($config['queue_jobs'] ?? false) === true; $this->traceQueueJobsAsTransactions = ($config['queue_job_transactions'] ?? false) === true; @@ -174,6 +174,12 @@ protected function queryExecutedHandler(DatabaseEvents\QueryExecuted $query): vo $context->setStartTimestamp(microtime(true) - $query->time / 1000); $context->setEndTimestamp($context->getStartTimestamp() + $query->time / 1000); + if ($this->traceSqlBindings) { + $context->setData(array_merge($context->getData(), [ + 'db.sql.bindings' => $query->bindings + ])); + } + if ($this->traceSqlQueryOrigins) { $queryOrigin = $this->resolveQueryOriginFromBacktrace(); @@ -184,12 +190,6 @@ protected function queryExecutedHandler(DatabaseEvents\QueryExecuted $query): vo } } - if ($this->recordSqlBindings) { - $context->setData(array_merge($context->getData(), [ - 'db.sql.bindings' => $query->bindings - ])); - } - $parentSpan->startChild($context); } From 7e8d37f2c33751413d0351f3698cbea0172acc46 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Wed, 22 Nov 2023 09:45:20 +0100 Subject: [PATCH 4/4] Move and cleanup tests --- .../Features/DatabaseIntegrationTest.php | 34 +++++++++- test/Sentry/TestCase.php | 13 ---- test/Sentry/Tracing/EventHandlerTest.php | 64 ------------------- 3 files changed, 32 insertions(+), 79 deletions(-) diff --git a/test/Sentry/Features/DatabaseIntegrationTest.php b/test/Sentry/Features/DatabaseIntegrationTest.php index 3c32aec0..64504bae 100644 --- a/test/Sentry/Features/DatabaseIntegrationTest.php +++ b/test/Sentry/Features/DatabaseIntegrationTest.php @@ -85,11 +85,41 @@ public function testSpanIsCreatedForSqliteConnectionQuery(): void $this->assertNull($span->getData()['server.port']); } - private function executeQueryAndRetrieveSpan(string $query): Span + public function testSqlBindingsAreRecordedWhenEnabled(): void + { + $this->resetApplicationWithConfig([ + 'sentry.tracing.sql_bindings' => true, + ]); + + $span = $this->executeQueryAndRetrieveSpan( + $query = 'SELECT %', + $bindings = ['1'] + ); + + $this->assertEquals($query, $span->getDescription()); + $this->assertEquals($bindings, $span->getData()['db.sql.bindings']); + } + + public function testSqlBindingsAreRecordedWhenDisabled(): void + { + $this->resetApplicationWithConfig([ + 'sentry.tracing.sql_bindings' => false, + ]); + + $span = $this->executeQueryAndRetrieveSpan( + $query = 'SELECT %', + ['1'] + ); + + $this->assertEquals($query, $span->getDescription()); + $this->assertFalse(isset($span->getData()['db.sql.bindings'])); + } + + private function executeQueryAndRetrieveSpan(string $query, array $bindings = []): Span { $transaction = $this->startTransaction(); - $this->dispatchLaravelEvent(new QueryExecuted($query, [], 123, DB::connection())); + $this->dispatchLaravelEvent(new QueryExecuted($query, $bindings, 123, DB::connection())); $spans = $transaction->getSpanRecorder()->getSpans(); diff --git a/test/Sentry/TestCase.php b/test/Sentry/TestCase.php index fe23e48f..b6766421 100644 --- a/test/Sentry/TestCase.php +++ b/test/Sentry/TestCase.php @@ -2,8 +2,6 @@ namespace Sentry\Laravel\Tests; -use Sentry\SentrySdk; -use Sentry\Tracing\Span; use Sentry\Tracing\Transaction; use Illuminate\Config\Repository; use Sentry\Tracing\TransactionContext; @@ -21,7 +19,6 @@ use Sentry\State\HubInterface; use Sentry\Laravel\ServiceProvider; use Orchestra\Testbench\TestCase as LaravelTestCase; -use Throwable; abstract class TestCase extends LaravelTestCase { @@ -158,16 +155,6 @@ protected function getLastSentryEvent(): ?Event return end(self::$lastSentryEvents)[0]; } - protected function getLastSentrySpan(): ?Span - { - try { - $spans = SentrySdk::getCurrentHub()->getSpan()->getSpanRecorder()->getSpans(); - return end($spans); - } catch (Throwable $throwable) { - return null; - } - } - protected function getLastEventSentryHint(): ?EventHint { if (empty(self::$lastSentryEvents)) { diff --git a/test/Sentry/Tracing/EventHandlerTest.php b/test/Sentry/Tracing/EventHandlerTest.php index 49209867..db4ceb31 100644 --- a/test/Sentry/Tracing/EventHandlerTest.php +++ b/test/Sentry/Tracing/EventHandlerTest.php @@ -2,9 +2,6 @@ namespace Sentry\Laravel\Tests\Tracing; -use Illuminate\Database\Connection; -use Illuminate\Database\Events\QueryExecuted; -use Mockery; use ReflectionClass; use RuntimeException; use Sentry\Laravel\Tests\TestCase; @@ -29,56 +26,6 @@ public function testAllMappedEventHandlersExist(): void ); } - public function testSqlBindingsAreRecordedWhenEnabled(): void - { - $this->resetApplicationWithConfig([ - 'sentry.traces_sample_rate' => 1, - 'sentry.tracing.sql_queries' => true, - 'sentry.tracing.sql_bindings' => true, - ]); - - $this->assertTrue($this->app['config']->get('sentry.tracing.sql_bindings')); - - $this->startTransaction(); - - $this->dispatchLaravelEvent(new QueryExecuted( - $query = 'SELECT * FROM spans WHERE bindings = ?;', - $bindings = ['1'], - 10, - $this->getMockedConnection() - )); - - $span = $this->getLastSentrySpan(); - - $this->assertEquals($query, $span->getDescription()); - $this->assertEquals($bindings, $span->getData()['db.sql.bindings']); - } - - public function testSqlBindingsAreRecordedWhenDisabled(): void - { - $this->resetApplicationWithConfig([ - 'sentry.traces_sample_rate' => 1, - 'sentry.tracing.sql_queries' => true, - 'sentry.tracing.sql_bindings' => false, - ]); - - $this->assertFalse($this->app['config']->get('sentry.tracing.sql_bindings')); - - $this->startTransaction(); - - $this->dispatchLaravelEvent(new QueryExecuted( - $query = 'SELECT * FROM spans WHERE bindings = ?;', - $bindings = ['1'], - 10, - $this->getMockedConnection() - )); - - $span = $this->getLastSentrySpan(); - - $this->assertEquals($query, $span->getDescription()); - $this->assertFalse(isset($span->getData()['db.sql.bindings'])); - } - private function tryAllEventHandlerMethods(array $methods): void { $handler = new EventHandler([]); @@ -100,15 +47,4 @@ private function getEventHandlerMapFromEventHandler() return $attributes['eventHandlerMap']; } - - private function getMockedConnection() - { - $mock = Mockery::mock(Connection::class); - $mock->shouldReceive('getName')->andReturn('test'); - $mock->shouldReceive('getDatabaseName')->andReturn('test'); - $mock->shouldReceive('getDriverName')->andReturn('mysql'); - $mock->shouldReceive('getConfig')->andReturn(['host' => '127.0.0.1', 'port' => 3306]); - - return $mock; - } }