facade/ignition#302: Fix memory leaks caused by log and query recorder

config/flare.php

@@ -32,6 +32,8 @@
         'report_query_bindings' => true,
         'report_view_data' => true,
         'grouping_type' => null,
+        'report_logs' => true,
+        'maximum_number_of_collected_logs' => 200,
     ],
 
     /*

src/IgnitionServiceProvider.php

@@ -90,8 +90,14 @@ public function boot()
             $this->setupQueue($this->app->get('queue'));
         }
 
-        $this->app->make(QueryRecorder::class)->register();
-        $this->app->make(LogRecorder::class)->register();
+        if (config('flare.reporting.report_logs')) {
+            $this->app->make(LogRecorder::class)->register();
+        }
+
+        if (config('flare.reporting.report_queries')) {
+            $this->app->make(QueryRecorder::class)->register();
+        }
+
         $this->app->make(DumpRecorder::class)->register();
     }
 
@@ -106,9 +112,12 @@ public function register()
             ->registerWhoopsHandler()
             ->registerIgnitionConfig()
             ->registerFlare()
-            ->registerLogRecorder()
             ->registerDumpCollector();
 
+        if (config('flare.reporting.report_logs')) {
+            $this->registerLogRecorder();
+        }
+
         if (config('flare.reporting.report_queries')) {
             $this->registerQueryRecorder();
         }
@@ -279,13 +288,14 @@ protected function getLogLevel(string $logLevelString): int
         return $logLevel;
     }
 
-    protected function registerLogRecorder()
+    protected function registerLogRecorder(): self
     {
-        $logCollector = $this->app->make(LogRecorder::class);
-
-        $this->app->singleton(LogRecorder::class);
-
-        $this->app->instance(LogRecorder::class, $logCollector);
+        $this->app->singleton(LogRecorder::class, function (Application $app): LogRecorder {
+            return new LogRecorder(
+                $app,
+                $app->get('config')->get('flare.reporting.maximum_number_of_collected_logs')
+            );
+        });
 
         return $this;
     }
@@ -319,30 +329,42 @@ protected function registerCommands()
         return $this;
     }
 
-    protected function registerQueryRecorder()
+    protected function registerQueryRecorder(): self
     {
-        $queryCollector = $this->app->make(QueryRecorder::class);
-
-        $this->app->singleton(QueryRecorder::class);
-
-        $this->app->instance(QueryRecorder::class, $queryCollector);
+        $this->app->singleton(QueryRecorder::class, function (Application $app): QueryRecorder {
+            return new QueryRecorder(
+                $app,
+                $app->get('config')->get('flare.reporting.report_query_bindings'),
+                $app->get('config')->get('flare.reporting.maximum_number_of_collected_logs')
+            );
+        });
 
         return $this;
     }
 
     protected function registerBuiltInMiddleware()
     {
-        $middleware = collect([
+        $middlewares = [
             SetNotifierName::class,
             AddEnvironmentInformation::class,
-            AddLogs::class,
-            AddDumps::class,
-            AddQueries::class,
-            AddSolutions::class,
-        ])
-        ->map(function (string $middlewareClass) {
-            return $this->app->make($middlewareClass);
-        });
+        ];
+
+        if (config('flare.reporting.report_logs')) {
+            $middlewares[] = AddLogs::class;
+        }
+
+        $middlewares[] = AddDumps::class;
+
+        if (config('flare.reporting.report_queries')) {
+            $middlewares[] = AddQueries::class;
+        }
+
+        $middlewares[] = AddSolutions::class;
+
+        $middleware = collect($middlewares)
+            ->map(function (string $middlewareClass) {
+                return $this->app->make($middlewareClass);
+            });
 
         if (config('flare.reporting.collect_git_information')) {
             $middleware[] = (new AddGitInformation());
@@ -436,12 +458,14 @@ protected function setupQueue(QueueManager $queue)
         $queue->looping(function () {
             $this->app->get(Flare::class)->reset();
 
+            if (config('flare.reporting.report_logs')) {
+                $this->app->make(LogRecorder::class)->reset();
+            }
+
             if (config('flare.reporting.report_queries')) {
                 $this->app->make(QueryRecorder::class)->reset();
             }
 
-            $this->app->make(LogRecorder::class)->reset();
-
             $this->app->make(DumpRecorder::class)->reset();
         });
     }

src/LogRecorder/LogRecorder.php

@@ -14,9 +14,13 @@ class LogRecorder
     /** @var \Illuminate\Contracts\Foundation\Application */
     protected $app;
 
-    public function __construct(Application $app)
+    /** @var int|null */
+    private $maxLogs;
+
+    public function __construct(Application $app, ?int $maxLogs = null)
     {
         $this->app = $app;
+        $this->maxLogs = $maxLogs;
     }
 
     public function register(): self
@@ -33,6 +37,10 @@ public function record(MessageLogged $event): void
         }
 
         $this->logMessages[] = LogMessage::fromMessageLoggedEvent($event);
+
+        if (is_int($this->maxLogs)) {
+            $this->logMessages = array_slice($this->logMessages, -$this->maxLogs);
+        }
     }
 
     public function getLogMessages(): array
@@ -68,4 +76,16 @@ public function reset(): void
     {
         $this->logMessages = [];
     }
+
+    public function getMaxLogs(): ?int
+    {
+        return $this->maxLogs;
+    }
+
+    public function setMaxLogs(?int $maxLogs): self
+    {
+        $this->maxLogs = $maxLogs;
+
+        return $this;
+    }
 }

src/QueryRecorder/QueryRecorder.php

@@ -13,9 +13,20 @@ class QueryRecorder
     /** @var \Illuminate\Contracts\Foundation\Application */
     protected $app;
 
-    public function __construct(Application $app)
-    {
+    /** @var bool */
+    private $reportBindings;
+
+    /** @var int|null */
+    private $maxQueries;
+
+    public function __construct(
+        Application $app,
+        bool $reportBindings = true,
+        ?int $maxQueries = null
+    ) {
         $this->app = $app;
+        $this->reportBindings = $reportBindings;
+        $this->maxQueries = $maxQueries;
     }
 
     public function register()
@@ -27,13 +38,11 @@ public function register()
 
     public function record(QueryExecuted $queryExecuted)
     {
-        $maximumQueries = $this->app['config']->get('flare.reporting.maximum_number_of_collected_queries', 200);
+        $this->queries[] = Query::fromQueryExecutedEvent($queryExecuted, $this->reportBindings);
 
-        $reportBindings = $this->app['config']->get('flare.reporting.report_query_bindings', true);
-
-        $this->queries[] = Query::fromQueryExecutedEvent($queryExecuted, $reportBindings);
-
-        $this->queries = array_slice($this->queries, $maximumQueries * -1, $maximumQueries);
+        if (is_int($this->maxQueries)) {
+            $this->queries = array_slice($this->queries, -$this->maxQueries);
+        }
     }
 
     public function getQueries(): array
@@ -51,4 +60,28 @@ public function reset()
     {
         $this->queries = [];
     }
+
+    public function getReportBindings(): bool
+    {
+        return $this->reportBindings;
+    }
+
+    public function setReportBindings(bool $reportBindings): self
+    {
+        $this->reportBindings = $reportBindings;
+
+        return $this;
+    }
+
+    public function getMaxQueries(): ?int
+    {
+        return $this->maxQueries;
+    }
+
+    public function setMaxQueries(?int $maxQueries): self
+    {
+        $this->maxQueries = $maxQueries;
+
+        return $this;
+    }
 }

tests/LogRecorder/LogRecorderTest.php

@@ -0,0 +1,71 @@
+<?php
+
+namespace Facade\Ignition\Tests\LogRecorder;
+
+use Exception;
+use Facade\Ignition\LogRecorder\LogRecorder;
+use Facade\Ignition\Tests\TestCase;
+use Illuminate\Log\Events\MessageLogged;
+
+class LogRecorderTest extends TestCase
+{
+    /** @test */
+    public function it_limits_the_amount_of_recorded_logs()
+    {
+        $recorder = new LogRecorder($this->app, 200);
+
+        foreach (range(1, 400) as $i) {
+            $log = new MessageLogged('info', 'test ' . $i, []);
+            $recorder->record($log);
+        }
+
+        $this->assertCount(200, $recorder->getLogMessages());
+        $this->assertSame('test 201', $recorder->getLogMessages()[0]['message']);
+    }
+
+    /** @test */
+    public function it_does_not_limit_the_amount_of_recorded_queries()
+    {
+        $recorder = new LogRecorder($this->app);
+
+        foreach (range(1, 400) as $i) {
+            $log = new MessageLogged('info', 'test ' . $i, []);
+            $recorder->record($log);
+        }
+
+        $this->assertCount(400, $recorder->getLogMessages());
+        $this->assertSame('test 1', $recorder->getLogMessages()[0]['message']);
+    }
+
+    /** @test */
+    public function it_does_not_record_log_containing_an_exception()
+    {
+        $recorder = new LogRecorder($this->app);
+
+        $log = new MessageLogged('info', 'test 1', ['exception' => new Exception('test')]);
+        $recorder->record($log);
+        $log = new MessageLogged('info', 'test 2', []);
+        $recorder->record($log);
+
+        $this->assertCount(1, $recorder->getLogMessages());
+        $this->assertSame('test 2', $recorder->getLogMessages()[0]['message']);
+    }
+
+    /** @test */
+    public function it_does_not_ignore_log_if_exception_key_does_not_contain_exception()
+    {
+        $recorder = new LogRecorder($this->app);
+
+        $log = new MessageLogged('info', 'test 1', ['exception' => 'test']);
+        $recorder->record($log);
+        $log = new MessageLogged('info', 'test 2', []);
+        $recorder->record($log);
+
+        $this->assertCount(2, $recorder->getLogMessages());
+        $this->assertSame('test 1', $recorder->getLogMessages()[0]['message']);
+        $this->assertSame('test 2', $recorder->getLogMessages()[1]['message']);
+        $this->assertIsArray($recorder->getLogMessages()[0]['context']);
+        $this->assertArrayHasKey('exception', $recorder->getLogMessages()[0]['context']);
+        $this->assertSame('test', $recorder->getLogMessages()[0]['context']['exception']);
+    }
+}

tests/QueryRecorder/QueryRecorderTest.php

@@ -12,7 +12,7 @@ class QueryRecorderTest extends TestCase
     /** @test */
     public function it_limits_the_amount_of_recorded_queries()
     {
-        $recorder = new QueryRecorder($this->app);
+        $recorder = new QueryRecorder($this->app, true, 200);
         $connection = app(Connection::class);
 
         foreach (range(1, 400) as $i) {
@@ -23,4 +23,49 @@ public function it_limits_the_amount_of_recorded_queries()
         $this->assertCount(200, $recorder->getQueries());
         $this->assertSame('query 201', $recorder->getQueries()[0]['sql']);
     }
+
+    /** @test */
+    public function it_does_not_limit_the_amount_of_recorded_queries()
+    {
+        $recorder = new QueryRecorder($this->app, true);
+        $connection = app(Connection::class);
+
+        foreach (range(1, 400) as $i) {
+            $query = new QueryExecuted('query '.$i, [], time(), $connection);
+            $recorder->record($query);
+        }
+
+        $this->assertCount(400, $recorder->getQueries());
+        $this->assertSame('query 1', $recorder->getQueries()[0]['sql']);
+    }
+
+    /** @test */
+    public function it_records_bindings()
+    {
+        $recorder = new QueryRecorder($this->app, true);
+        $connection = app(Connection::class);
+
+        $query = new QueryExecuted('query 1', ['abc' => 123], time(), $connection);
+        $recorder->record($query);
+
+        $this->assertCount(1, $recorder->getQueries());
+        $this->assertSame('query 1', $recorder->getQueries()[0]['sql']);
+        $this->assertIsArray($recorder->getQueries()[0]['bindings']);
+        $this->assertSame('query 1', $recorder->getQueries()[0]['sql']);
+        $this->assertSame(123, $recorder->getQueries()[0]['bindings']['abc']);
+    }
+
+    /** @test */
+    public function it_does_not_record_bindings()
+    {
+        $recorder = new QueryRecorder($this->app, false);
+        $connection = app(Connection::class);
+
+        $query = new QueryExecuted('query 1', ['abc' => 123], time(), $connection);
+        $recorder->record($query);
+
+        $this->assertCount(1, $recorder->getQueries());
+        $this->assertSame('query 1', $recorder->getQueries()[0]['sql']);
+        $this->assertNull($recorder->getQueries()[0]['bindings']);
+    }
 }