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

Always start performance tracing #600

Merged
merged 8 commits into from Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Remove incorrect checks if performance tracing should be enabled and rely on the transaction sampling decision instead (#600)

## 3.0.0

**New features**
Expand Down
4 changes: 2 additions & 2 deletions composer.json
Expand Up @@ -23,8 +23,8 @@
"require": {
"php": "^7.2 | ^8.0",
"illuminate/support": "^6.0 | ^7.0 | ^8.0 | ^9.0",
"sentry/sentry": "^3.9",
"sentry/sdk": "^3.1",
"sentry/sentry": "^3.10",
"sentry/sdk": "^3.3",
"symfony/psr-http-message-bridge": "^1.0 | ^2.0",
"nyholm/psr7": "^1.0"
},
Expand Down
1 change: 0 additions & 1 deletion src/Sentry/Laravel/EventHandler.php
Expand Up @@ -14,7 +14,6 @@
use Illuminate\Http\Request;
use Illuminate\Log\Events as LogEvents;
use Illuminate\Queue\Events as QueueEvents;
use Illuminate\Queue\QueueManager;
use Illuminate\Routing\Events as RoutingEvents;
use Laravel\Octane\Events as Octane;
use Laravel\Sanctum\Events as Sanctum;
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/Laravel/ServiceProvider.php
Expand Up @@ -238,7 +238,7 @@ private function resolveIntegrationsFromUserConfig(): array

$enableDefaultTracingIntegrations = $userConfig['tracing']['default_integrations'] ?? true;

if ($enableDefaultTracingIntegrations && $this->couldHavePerformanceTracingEnabled()) {
if ($enableDefaultTracingIntegrations) {
$integrationsToResolve = array_merge($integrationsToResolve, TracingServiceProvider::DEFAULT_INTEGRATIONS);
}

Expand Down
10 changes: 5 additions & 5 deletions src/Sentry/Laravel/Tracing/EventHandler.php
Expand Up @@ -250,6 +250,7 @@ protected function transactionBeginningHandler(DatabaseEvents\TransactionBeginni
{
$parentSpan = SentrySdk::getCurrentHub()->getSpan();

// If there is no tracing span active there is no need to handle the event
if ($parentSpan === null) {
return;
}
Expand Down Expand Up @@ -284,6 +285,7 @@ protected function httpClientRequestSendingHandler(HttpClientEvents\RequestSendi
{
$parentSpan = SentrySdk::getCurrentHub()->getSpan();

// If there is no tracing span active there is no need to handle the event
if ($parentSpan === null) {
return;
}
Expand Down Expand Up @@ -387,12 +389,10 @@ private function afterQueuedJob(?SpanStatus $status = null): void
{
$span = $this->popSpan();

if ($span === null) {
return;
if ($span !== null) {
$span->finish();
$span->setStatus($status);
}

$span->setStatus($status);
$span->finish();
}

private function pushSpan(Span $span): void
Expand Down
42 changes: 26 additions & 16 deletions src/Sentry/Laravel/Tracing/Middleware.php
Expand Up @@ -62,26 +62,29 @@ public function handle(Request $request, Closure $next)
*/
public function terminate(Request $request, $response): void
{
if ($this->transaction !== null && app()->bound(HubInterface::class)) {
// We stop here if a route has not been matched unless we are configured to trace missing routes
if (config('sentry.tracing.missing_routes', false) === false && $request->route() === null) {
return;
}
// If there is no transaction or the HubInterface is not bound in the container there is nothing for us to do
if ($this->transaction === null || !app()->bound(HubInterface::class)) {
return;
}

if ($this->appSpan !== null) {
$this->appSpan->finish();
}
// We stop here if a route has not been matched unless we are configured to trace missing routes
if (config('sentry.tracing.missing_routes', false) === false && $request->route() === null) {
return;
}

// Make sure we set the transaction and not have a child span in the Sentry SDK
// If the transaction is not on the scope during finish, the trace.context is wrong
SentrySdk::getCurrentHub()->setSpan($this->transaction);
if ($this->appSpan !== null) {
$this->appSpan->finish();
}

if ($response instanceof SymfonyResponse) {
$this->hydrateResponseData($response);
}
// Make sure we set the transaction and not have a child span in the Sentry SDK
// If the transaction is not on the scope during finish, the trace.context is wrong
SentrySdk::getCurrentHub()->setSpan($this->transaction);

$this->transaction->finish();
if ($response instanceof SymfonyResponse) {
$this->hydrateResponseData($response);
}

$this->transaction->finish();
}

/**
Expand Down Expand Up @@ -119,7 +122,14 @@ private function startTransaction(Request $request, HubInterface $sentry): void
'method' => strtoupper($request->method()),
]);

$this->transaction = $sentry->startTransaction($context);
$transaction = $sentry->startTransaction($context);

// If this transaction is not sampled, don't set it either and stop doing work from this point on
if (!$transaction->getSampled()) {
return;
}

$this->transaction = $transaction;

// Setting the Transaction on the Hub
SentrySdk::getCurrentHub()->setSpan($this->transaction);
Expand Down
Expand Up @@ -12,7 +12,9 @@ protected function wrapRouteDispatch(callable $dispatch, Route $route)
{
$parentSpan = SentrySdk::getCurrentHub()->getSpan();

// When there is no active transaction we can skip doing anything and just immediately return the callable
stayallive marked this conversation as resolved.
Show resolved Hide resolved
// When there is no span we can skip creating
// the span and just immediately return with the
// callable result because there is no transaction.
if ($parentSpan === null) {
return $dispatch();
}
Expand Down
33 changes: 18 additions & 15 deletions src/Sentry/Laravel/Tracing/ServiceProvider.php
Expand Up @@ -26,22 +26,29 @@ class ServiceProvider extends BaseServiceProvider

public function boot(): void
{
if ($this->hasDsnSet() && $this->couldHavePerformanceTracingEnabled()) {
$tracingConfig = $this->getUserConfig()['tracing'] ?? [];
// If there is no DSN set we register nothing since it's impossible for us to send traces without a DSN set
if (!$this->hasDsnSet()) {
return;
}

$this->bindEvents($tracingConfig);
$this->app->booted(function () {
$this->app->make(Middleware::class)->setBootedTimestamp();
});

$this->bindViewEngine($tracingConfig);
$tracingConfig = $this->getUserConfig()['tracing'] ?? [];

$this->decorateRoutingDispatchers();
$this->bindEvents($tracingConfig);

if ($this->app->bound(HttpKernelInterface::class)) {
/** @var \Illuminate\Foundation\Http\Kernel $httpKernel */
$httpKernel = $this->app->make(HttpKernelInterface::class);
$this->bindViewEngine($tracingConfig);

if ($httpKernel instanceof HttpKernel) {
$httpKernel->prependMiddleware(Middleware::class);
}
$this->decorateRoutingDispatchers();

if ($this->app->bound(HttpKernelInterface::class)) {
/** @var \Illuminate\Foundation\Http\Kernel $httpKernel */
$httpKernel = $this->app->make(HttpKernelInterface::class);

if ($httpKernel instanceof HttpKernel) {
$httpKernel->prependMiddleware(Middleware::class);
}
}
}
Expand All @@ -58,10 +65,6 @@ public function register(): void

return new BacktraceHelper($options, new RepresentationSerializer($options));
});

$this->app->booted(function () {
$this->app->make(Middleware::class)->setBootedTimestamp();
});
}

private function bindEvents(array $tracingConfig): void
Expand Down