diff --git a/CHANGELOG.md b/CHANGELOG.md index 35fb5a55..dd158488 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,11 +3,14 @@ ## Unreleased - Drop support for Laravel Lumen (#579) +- Set the tracing transaction name on the `Illuminate\Routing\Events\RouteMatched` instead of at the end of the request (#580) +- Remove `Sentry\Integration::extractNameForRoute()`, it's alternative `Sentry\Integration::extractNameAndSourceForRoute()` is marked as `@internal` (#580) - Drop support for Laravel 5.x (#581) ## 2.14.1 - Fix not setting the correct SDK ID and version when running the `sentry:test` command (#582) +- Transaction names now only show the parameterized URL (`/some/{route}`) instead of the route name or controller class (#583) ## 2.14.0 diff --git a/config/sentry.php b/config/sentry.php index b0e4b700..08c918bf 100644 --- a/config/sentry.php +++ b/config/sentry.php @@ -45,6 +45,9 @@ // Indicates if the tracing integrations supplied by Sentry should be loaded 'default_integrations' => true, + + // Indicates that requests without a matching route should be traced + 'missing_routes' => false, ], // @see: https://docs.sentry.io/platforms/php/configuration/options/#send-default-pii @@ -52,6 +55,4 @@ 'traces_sample_rate' => (float)(env('SENTRY_TRACES_SAMPLE_RATE', 0.0)), - 'controllers_base_namespace' => env('SENTRY_CONTROLLERS_BASE_NAMESPACE', 'App\\Http\\Controllers'), - ]; diff --git a/src/Sentry/Laravel/Integration.php b/src/Sentry/Laravel/Integration.php index 6254d84b..8f683781 100644 --- a/src/Sentry/Laravel/Integration.php +++ b/src/Sentry/Laravel/Integration.php @@ -6,6 +6,7 @@ use Illuminate\Support\Str; use Sentry\SentrySdk; use Sentry\Tracing\Span; +use Sentry\Tracing\Transaction; use Sentry\Tracing\TransactionSource; use function Sentry\addBreadcrumb; use function Sentry\configureScope; @@ -21,11 +22,6 @@ class Integration implements IntegrationInterface */ private static $transaction; - /** - * @var null|string - */ - private static $baseControllerNamespace; - /** * {@inheritdoc} */ @@ -94,14 +90,6 @@ public static function setTransaction(?string $transaction): void self::$transaction = $transaction; } - /** - * @param null|string $namespace - */ - public static function setControllersBaseNamespace(?string $namespace): void - { - self::$baseControllerNamespace = $namespace !== null ? trim($namespace, '\\') : null; - } - /** * Block until all async events are processed for the HTTP transport. * @@ -117,21 +105,6 @@ public static function flushEvents(): void } } - /** - * Extract the readable name for a route. - * - * @param \Illuminate\Routing\Route $route - * - * @return string - * - * @internal This helper is used in various places to extra meaninful info from a Laravel Route object. - * @deprecated This will be removed in version 3.0, use `extractNameAndSourceForRoute` instead. - */ - public static function extractNameForRoute(Route $route): string - { - return self::extractNameAndSourceForRoute($route)[0]; - } - /** * Extract the readable name for a route and the transaction source for where that route name came from. * @@ -143,75 +116,10 @@ public static function extractNameForRoute(Route $route): string */ public static function extractNameAndSourceForRoute(Route $route): array { - $source = null; - $routeName = null; - - // some.action (route name/alias) - if ($route->getName()) { - $source = TransactionSource::component(); - $routeName = self::extractNameForNamedRoute($route->getName()); - } - - // Some\Controller@someAction (controller action) - if (empty($routeName) && $route->getActionName()) { - $source = TransactionSource::component(); - $routeName = self::extractNameForActionRoute($route->getActionName()); - } - - // /some/{action} // Fallback to the route uri (with parameter placeholders) - if (empty($routeName) || $routeName === 'Closure') { - $source = TransactionSource::route(); - $routeName = '/' . ltrim($route->uri(), '/'); - } - - return [$routeName, $source]; - } - - /** - * Take a route name and return it only if it's a usable route name. - * - * @param string $name - * - * @return string|null - */ - private static function extractNameForNamedRoute(string $name): ?string - { - // Laravel 7 route caching generates a route names if the user didn't specify one - // theirselfs to optimize route matching. These route names are useless to the - // developer so if we encounter a generated route name we discard the value - if (Str::contains($name, 'generated::')) { - return null; - } - - // If the route name ends with a `.` we assume an incomplete group name prefix - // we discard this value since it will most likely not mean anything to the - // developer and will be duplicated by other unnamed routes in the group - if (Str::endsWith($name, '.')) { - return null; - } - - return $name; - } - - /** - * Take a controller action and strip away the base namespace if needed. - * - * @param string $action - * - * @return string - */ - private static function extractNameForActionRoute(string $action): string - { - $routeName = ltrim($action, '\\'); - - $baseNamespace = self::$baseControllerNamespace ?? ''; - - if (empty($baseNamespace)) { - return $routeName; - } - - // Strip away the base namespace from the action name - return ltrim(Str::after($routeName, $baseNamespace), '\\'); + return [ + '/' . ltrim($route->uri(), '/'), + TransactionSource::route() + ]; } /** @@ -258,6 +166,18 @@ public static function sentryBaggageMeta(): string return sprintf('', $span->toBaggage()); } + /** + * Get the current active tracing span from the scope. + * + * @return \Sentry\Tracing\Transaction|null + * + * @internal This is used internally as an easy way to retrieve the current active transaction. + */ + public static function currentTransaction(): ?Transaction + { + return SentrySdk::getCurrentHub()->getTransaction(); + } + /** * Get the current active tracing span from the scope. * diff --git a/src/Sentry/Laravel/ServiceProvider.php b/src/Sentry/Laravel/ServiceProvider.php index 9edb5015..5b8bf12a 100644 --- a/src/Sentry/Laravel/ServiceProvider.php +++ b/src/Sentry/Laravel/ServiceProvider.php @@ -33,7 +33,8 @@ class ServiceProvider extends BaseServiceProvider 'integrations', // This is kept for backwards compatibility and can be dropped in a future breaking release 'breadcrumbs.sql_bindings', - // The base namespace for controllers to strip of the beginning of controller class names + + // This config option is no longer in use but to prevent errors when upgrading we leave it here to be discarded 'controllers_base_namespace', ]; @@ -125,12 +126,6 @@ protected function registerArtisanCommands(): void */ protected function configureAndRegisterClient(): void { - $userConfig = $this->getUserConfig(); - - if (isset($userConfig['controllers_base_namespace'])) { - Integration::setControllersBaseNamespace($userConfig['controllers_base_namespace']); - } - $this->app->bind(ClientBuilderInterface::class, function () { $basePath = base_path(); $userConfig = $this->getUserConfig(); @@ -161,7 +156,7 @@ protected function configureAndRegisterClient(): void return $clientBuilder; }); - $this->app->singleton(HubInterface::class, function ($app) { + $this->app->singleton(HubInterface::class, function () { /** @var \Sentry\ClientBuilderInterface $clientBuilder */ $clientBuilder = $this->app->make(ClientBuilderInterface::class); @@ -169,7 +164,7 @@ protected function configureAndRegisterClient(): void $userIntegrations = $this->resolveIntegrationsFromUserConfig(); - $options->setIntegrations(function (array $integrations) use ($options, $userIntegrations, $app) { + $options->setIntegrations(function (array $integrations) use ($options, $userIntegrations) { if ($options->hasDefaultIntegrations()) { // Remove the default error and fatal exception listeners to let Laravel handle those // itself. These event are still bubbling up through the documented changes in the users diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index bcba08b7..d971d32a 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -7,9 +7,11 @@ use Illuminate\Contracts\Container\Container; use Illuminate\Contracts\Events\Dispatcher; use Illuminate\Database\Events as DatabaseEvents; +use Illuminate\Http\Client\Events as HttpClientEvents; use Illuminate\Queue\Events as QueueEvents; use Illuminate\Queue\Queue; use Illuminate\Queue\QueueManager; +use Illuminate\Routing\Events as RoutingEvents; use RuntimeException; use Sentry\Laravel\Integration; use Sentry\SentrySdk; @@ -30,7 +32,11 @@ class EventHandler * @var array */ protected static $eventHandlerMap = [ + RoutingEvents\RouteMatched::class => 'routeMatched', DatabaseEvents\QueryExecuted::class => 'queryExecuted', + HttpClientEvents\RequestSending::class => 'httpClientRequestSending', + HttpClientEvents\ResponseReceived::class => 'httpClientResponseReceived', + HttpClientEvents\ConnectionFailed::class => 'httpClientConnectionFailed', ]; /** @@ -93,6 +99,20 @@ class EventHandler */ private $currentQueueJobSpan; + /** + * Holds a reference to the parent http client request span. + * + * @var \Sentry\Tracing\Span|null + */ + private $parentHttpClientRequestSpan; + + /** + * Holds a reference to the current http client request span. + * + * @var \Sentry\Tracing\Span|null + */ + private $currentHttpClientRequestSpan; + /** * The backtrace helper. * @@ -122,6 +142,7 @@ public function __construct(Container $container, BacktraceHelper $backtraceHelp /** * Attach all event handlers. * + * @uses self::routeMatchedHandler() * @uses self::queryExecutedHandler() */ public function subscribe(): void @@ -202,6 +223,20 @@ public function __call(string $method, array $arguments) } } + protected function routeMatchedHandler(RoutingEvents\RouteMatched $match): void + { + $transaction = Integration::currentTransaction(); + + if ($transaction === null) { + return; + } + + [$transactionName, $transactionSource] = Integration::extractNameAndSourceForRoute($match->route); + + $transaction->setName($transactionName); + $transaction->getMetadata()->setSource($transactionSource); + } + protected function queryExecutedHandler(DatabaseEvents\QueryExecuted $query): void { if (!$this->traceSqlQueries) { @@ -250,6 +285,56 @@ private function resolveQueryOriginFromBacktrace(): ?string return "{$filePath}:{$firstAppFrame->getLine()}"; } + protected function httpClientRequestSendingHandler(HttpClientEvents\RequestSending $event): void + { + $parentSpan = Integration::currentTracingSpan(); + + if ($parentSpan === null) { + return; + } + + $context = new SpanContext; + + $context->setOp('http.client'); + $context->setDescription($event->request->method() . ' ' . $event->request->url()); + $context->setStartTimestamp(microtime(true)); + + $this->currentHttpClientRequestSpan = $parentSpan->startChild($context); + + $this->parentHttpClientRequestSpan = $parentSpan; + + SentrySdk::getCurrentHub()->setSpan($this->currentHttpClientRequestSpan); + } + + protected function httpClientResponseReceivedHandler(HttpClientEvents\ResponseReceived $event): void + { + if ($this->currentHttpClientRequestSpan !== null) { + $this->currentHttpClientRequestSpan->setHttpStatus($event->response->status()); + $this->afterHttpClientRequest(); + } + } + + protected function httpClientConnectionFailedHandler(HttpClientEvents\ConnectionFailed $event): void + { + if ($this->currentHttpClientRequestSpan !== null) { + $this->currentHttpClientRequestSpan->setStatus(SpanStatus::internalError()); + $this->afterHttpClientRequest(); + } + } + + private function afterHttpClientRequest(): void + { + if ($this->currentHttpClientRequestSpan === null) { + return; + } + + $this->currentHttpClientRequestSpan->finish(); + $this->currentHttpClientRequestSpan = null; + + SentrySdk::getCurrentHub()->setSpan($this->parentHttpClientRequestSpan); + $this->parentHttpClientRequestSpan = null; + } + protected function queueJobProcessingHandler(QueueEvents\JobProcessing $event): void { $parentSpan = Integration::currentTracingSpan(); @@ -278,24 +363,18 @@ protected function queueJobProcessingHandler(QueueEvents\JobProcessing $event): $context = new SpanContext; } + $resolvedJobName = $event->job->resolveName(); + $job = [ 'job' => $event->job->getName(), 'queue' => $event->job->getQueue(), + 'resolved' => $event->job->resolveName(), 'attempts' => $event->job->attempts(), 'connection' => $event->connectionName, ]; - // Resolve name exists only from Laravel 5.3+ - $resolvedJobName = method_exists($event->job, 'resolveName') - ? $event->job->resolveName() - : null; - - if ($resolvedJobName !== null) { - $job['resolved'] = $resolvedJobName; - } - if ($context instanceof TransactionContext) { - $context->setName($resolvedJobName ?? $event->job->getName()); + $context->setName($resolvedJobName); $context->setSource(TransactionSource::task()); } diff --git a/src/Sentry/Laravel/Tracing/Middleware.php b/src/Sentry/Laravel/Tracing/Middleware.php index 460ec941..33d2afe2 100644 --- a/src/Sentry/Laravel/Tracing/Middleware.php +++ b/src/Sentry/Laravel/Tracing/Middleware.php @@ -4,15 +4,13 @@ use Closure; use Illuminate\Http\Request; -use Illuminate\Routing\Route; -use Sentry\Laravel\Integration; use Sentry\SentrySdk; use Sentry\State\HubInterface; use Sentry\Tracing\Span; use Sentry\Tracing\SpanContext; use Sentry\Tracing\TransactionContext; use Sentry\Tracing\TransactionSource; -use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpFoundation\Response as SymfonyResponse; class Middleware { @@ -45,7 +43,7 @@ class Middleware * * @return mixed */ - public function handle($request, Closure $next) + public function handle(Request $request, Closure $next) { if (app()->bound(HubInterface::class)) { $this->startTransaction($request, app(HubInterface::class)); @@ -57,14 +55,19 @@ public function handle($request, Closure $next) /** * Handle the application termination. * - * @param \Illuminate\Http\Request $request - * @param \Symfony\Component\HttpFoundation\Response $response + * @param \Illuminate\Http\Request $request + * @param mixed $response * * @return void */ - public function terminate($request, $response): void + 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 ($this->appSpan !== null) { $this->appSpan->finish(); } @@ -73,11 +76,7 @@ public function terminate($request, $response): void // If the transaction is not on the scope during finish, the trace.context is wrong SentrySdk::getCurrentHub()->setSpan($this->transaction); - if ($request instanceof Request) { - $this->hydrateRequestData($request); - } - - if ($response instanceof Response) { + if ($response instanceof SymfonyResponse) { $this->hydrateResponseData($response); } @@ -108,12 +107,17 @@ private function startTransaction(Request $request, HubInterface $sentry): void $request->header('baggage', '') ); + $requestPath = '/' . ltrim($request->path(), '/'); + $context->setOp('http.server'); + $context->setName($requestPath); + $context->setSource(TransactionSource::url()); + $context->setStartTimestamp($requestStartTime); + $context->setData([ - 'url' => '/' . ltrim($request->path(), '/'), + 'url' => $requestPath, 'method' => strtoupper($request->method()), ]); - $context->setStartTimestamp($requestStartTime); $this->transaction = $sentry->startTransaction($context); @@ -122,7 +126,7 @@ private function startTransaction(Request $request, HubInterface $sentry): void $bootstrapSpan = $this->addAppBootstrapSpan($request); - $appContextStart = new SpanContext(); + $appContextStart = new SpanContext; $appContextStart->setOp('middleware.handle'); $appContextStart->setStartTimestamp($bootstrapSpan ? $bootstrapSpan->getEndTimestamp() : microtime(true)); @@ -143,7 +147,7 @@ private function addAppBootstrapSpan(Request $request): ?Span return null; } - $spanContextStart = new SpanContext(); + $spanContextStart = new SpanContext; $spanContextStart->setOp('app.bootstrap'); $spanContextStart->setStartTimestamp($laravelStartTime); $spanContextStart->setEndTimestamp($this->bootedTimestamp); @@ -175,45 +179,8 @@ private function addBootDetailTimeSpans(Span $bootstrap): void $bootstrap->startChild($autoload); } - private function hydrateRequestData(Request $request): void - { - $route = $request->route(); - - if ($route instanceof Route) { - [$transactionName, $transactionSource] = Integration::extractNameAndSourceForRoute($route); - - $this->updateTransactionNameIfDefault($transactionName, $transactionSource); - - $this->transaction->setData([ - 'name' => $route->getName(), - 'action' => $route->getActionName(), - 'method' => $request->getMethod(), - ]); - } - - $this->updateTransactionNameIfDefault('/' . ltrim($request->path(), '/'), TransactionSource::url()); - } - - private function hydrateResponseData(Response $response): void + private function hydrateResponseData(SymfonyResponse $response): void { $this->transaction->setHttpStatus($response->getStatusCode()); } - - private function updateTransactionNameIfDefault(?string $name, ?TransactionSource $source): void - { - // Ignore empty names (and `null`) for caller convenience - if (empty($name)) { - return; - } - - // If the transaction already has a name other than the default - // ignore the new name, this will most occur if the user has set a - // transaction name themself before the application reaches this point - if ($this->transaction->getName() !== TransactionContext::DEFAULT_NAME) { - return; - } - - $this->transaction->setName($name); - $this->transaction->getMetadata()->setSource($source ?? TransactionSource::custom()); - } } diff --git a/test/Sentry/IntegrationTest.php b/test/Sentry/IntegrationTest.php index 2365931b..a2ed7af2 100644 --- a/test/Sentry/IntegrationTest.php +++ b/test/Sentry/IntegrationTest.php @@ -9,6 +9,7 @@ use Sentry\Event; use Sentry\Laravel\Integration; use Sentry\State\Scope; +use Sentry\Tracing\TransactionSource; use function Sentry\withScope; class IntegrationTest extends SentryLaravelTestCase @@ -88,36 +89,11 @@ public function testTransactionIsNotAppliedToEventWhenTransactionIsAlreadySet(): }); } - public function testExtractingNameForRouteWithName(): void - { - $route = (new Route('GET', '/foo', []))->name($routeName = 'foo-bar'); - - $this->assertSame($routeName, Integration::extractNameForRoute($route)); - } - - public function testExtractingNameForRouteWithAction(): void - { - $route = (new Route('GET', '/foo', [ - 'controller' => $controller = 'SomeController@someAction', - ])); - - $this->assertSame($controller, Integration::extractNameForRoute($route)); - } - public function testExtractingNameForRouteWithoutName(): void { $route = new Route('GET', $url = '/foo', []); - $this->assertSame($url, Integration::extractNameForRoute($route)); - } - - public function testExtractingNameForRouteWithActionAndName(): void - { - $route = (new Route('GET', '/foo', [ - 'controller' => 'SomeController@someAction', - ]))->name($routeName = 'foo-bar'); - - $this->assertSame($routeName, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } public function testExtractingNameForRouteWithAutoGeneratedName(): void @@ -125,26 +101,21 @@ public function testExtractingNameForRouteWithAutoGeneratedName(): void // We fake a generated name here, Laravel generates them each starting with `generated::` $route = (new Route('GET', $url = '/foo', []))->name('generated::KoAePbpBofo01ey4'); - $this->assertSame($url, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } public function testExtractingNameForRouteWithIncompleteGroupName(): void { $route = (new Route('GET', $url = '/foo', []))->name('group-name.'); - $this->assertSame($url, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } - public function testExtractingNameForRouteWithStrippedBaseNamespaceFromAction(): void + private function assetRouteNameAndSource(Route $route, string $expectedName, TransactionSource $expectedSource): void { - Integration::setControllersBaseNamespace('BaseNamespace'); - - $route = (new Route('GET', '/foo', [ - 'controller' => 'BaseNamespace\\SomeController@someAction', - ])); - - $this->assertSame('SomeController@someAction', Integration::extractNameForRoute($route)); + [$actualName, $actualSource] = Integration::extractNameAndSourceForRoute($route); - Integration::setControllersBaseNamespace(null); + $this->assertSame($expectedName, $actualName); + $this->assertSame($expectedSource, $actualSource); } }