From 9fba02f5c4850ae58d601babac9bd3984f5eb0c1 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Tue, 30 Jul 2024 14:04:39 +0200 Subject: [PATCH 1/2] Move delayed transaction finisher state out of middleware --- src/Sentry/Laravel/Tracing/Middleware.php | 36 +++++-------------- .../Laravel/Tracing/ServiceProvider.php | 2 ++ .../Laravel/Tracing/TransactionFinisher.php | 22 ++++++++++++ 3 files changed, 33 insertions(+), 27 deletions(-) create mode 100644 src/Sentry/Laravel/Tracing/TransactionFinisher.php diff --git a/src/Sentry/Laravel/Tracing/Middleware.php b/src/Sentry/Laravel/Tracing/Middleware.php index 57fbf3e6..eb7274aa 100644 --- a/src/Sentry/Laravel/Tracing/Middleware.php +++ b/src/Sentry/Laravel/Tracing/Middleware.php @@ -47,13 +47,6 @@ class Middleware */ private $continueAfterResponse; - /** - * Whether the terminating callback has been registered. - * - * @var bool - */ - private $registeredTerminatingCallback = false; - /** * Whether a defined route was matched in the application. * @@ -115,22 +108,11 @@ public function terminate(Request $request, $response): void } if ($this->continueAfterResponse) { - // Ensure we do not register the terminating callback multiple times since there is no point in doing so - if ($this->registeredTerminatingCallback) { - return; - } - - // We need to finish the transaction after the response has been sent to the client - // so we register a terminating callback to do so, this allows us to also capture - // spans that are created during the termination of the application like queue - // dispatched using dispatch(...)->afterResponse(). This middleware is called - // before the terminating callbacks so we are 99.9% sure to be the last one - // to run except if another terminating callback is registered after ours. - app()->terminating(function () { - $this->finishTransaction(); - }); - - $this->registeredTerminatingCallback = true; + // Resolving the transaction finisher class will register the terminating callback + // which is responsible for calling `finishTransaction`. We have registered the + // class as a singleton to keep the state in the container and away from here + // this way we ensure the callback is only registered once even for Octane. + app(TransactionFinisher::class); } else { $this->finishTransaction(); } @@ -220,12 +202,12 @@ private function addAppBootstrapSpan(): ?Span $span = $this->transaction->startChild($spanContextStart); - // Consume the booted timestamp, because we don't want to report the bootstrap span more than once - $this->bootedTimestamp = null; - // Add more information about the bootstrap section if possible $this->addBootDetailTimeSpans($span); + // Consume the booted timestamp, because we don't want to report the boot(strap) spans more than once + $this->bootedTimestamp = null; + return $span; } @@ -250,7 +232,7 @@ private function hydrateResponseData(SymfonyResponse $response): void $this->transaction->setHttpStatus($response->getStatusCode()); } - private function finishTransaction(): void + public function finishTransaction(): void { // We could end up multiple times here since we register a terminating callback so // double check if we have a transaction before trying to finish it since it could diff --git a/src/Sentry/Laravel/Tracing/ServiceProvider.php b/src/Sentry/Laravel/Tracing/ServiceProvider.php index 55e5c94b..f045d5f4 100644 --- a/src/Sentry/Laravel/Tracing/ServiceProvider.php +++ b/src/Sentry/Laravel/Tracing/ServiceProvider.php @@ -59,6 +59,8 @@ public function boot(): void public function register(): void { + $this->app->singleton(TransactionFinisher::class); + $this->app->singleton(Middleware::class, function () { $continueAfterResponse = ($this->getTracingConfig()['continue_after_response'] ?? true) === true; diff --git a/src/Sentry/Laravel/Tracing/TransactionFinisher.php b/src/Sentry/Laravel/Tracing/TransactionFinisher.php new file mode 100644 index 00000000..f632fb3b --- /dev/null +++ b/src/Sentry/Laravel/Tracing/TransactionFinisher.php @@ -0,0 +1,22 @@ +afterResponse(). This middleware is called + // before the terminating callbacks so we are 99.9% sure to be the last one + // to run except if another terminating callback is registered after ours. + app()->terminating(function () { + app(Middleware::class)->finishTransaction(); + }); + } +} From 5b24122e110bb32dda3da5c6e6d3572218cf9235 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 5 Aug 2024 16:29:32 +0200 Subject: [PATCH 2/2] Update comment --- src/Sentry/Laravel/Tracing/TransactionFinisher.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Laravel/Tracing/TransactionFinisher.php b/src/Sentry/Laravel/Tracing/TransactionFinisher.php index f632fb3b..d75a26ee 100644 --- a/src/Sentry/Laravel/Tracing/TransactionFinisher.php +++ b/src/Sentry/Laravel/Tracing/TransactionFinisher.php @@ -12,9 +12,15 @@ public function __construct() // We need to finish the transaction after the response has been sent to the client // so we register a terminating callback to do so, this allows us to also capture // spans that are created during the termination of the application like queue - // dispatched using dispatch(...)->afterResponse(). This middleware is called - // before the terminating callbacks so we are 99.9% sure to be the last one - // to run except if another terminating callback is registered after ours. + // dispatches using dispatch(...)->afterResponse() which are terminating + // callbacks themselfs just like we do below. + // + // This class is registered as a singleton in the container to ensure it's only + // instantiated once and the terminating callback is only registered once. + // + // It should be resolved from the container before the terminating callbacks are called. + // Good place is in the `terminate` callback of a middleware for example. + // This way we can be 99.9% sure to be the last ones to run. app()->terminating(function () { app(Middleware::class)->finishTransaction(); });