Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move delayed transaction finisher state out of middleware #936

Merged
merged 2 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 9 additions & 27 deletions src/Sentry/Laravel/Tracing/Middleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/Sentry/Laravel/Tracing/ServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
22 changes: 22 additions & 0 deletions src/Sentry/Laravel/Tracing/TransactionFinisher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

namespace Sentry\Laravel\Tracing;

/**
* @internal
*/
class TransactionFinisher
{
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.
stayallive marked this conversation as resolved.
Show resolved Hide resolved
app()->terminating(function () {
app(Middleware::class)->finishTransaction();
});
}
}