From 7dde913b519092cb5695b91c22f029da9e2ca9ec Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 6 Oct 2022 11:14:20 +0200 Subject: [PATCH 1/9] Drop Laravel Lumen support --- README.md | 1 + composer.json | 3 + src/Sentry/Laravel/Integration.php | 62 --------------- src/Sentry/Laravel/ServiceProvider.php | 10 +-- src/Sentry/Laravel/Tracing/Middleware.php | 17 +--- .../Laravel/Tracing/ServiceProvider.php | 14 +--- test/Sentry/IntegrationTest.php | 79 ------------------- 7 files changed, 11 insertions(+), 175 deletions(-) diff --git a/README.md b/README.md index 30be5cbd..f1bd372d 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,7 @@ try { ## Laravel Version Compatibility +- Laravel Lumen is supported until `2.14.x` - Laravel `<= 4.2.x` is supported until `0.8.x` - Laravel `<= 5.7.x` on PHP `<= 7.0` is supported until `0.11.x` - Laravel `>= 5.x.x` on PHP `>= 7.1` is supported in all versions diff --git a/composer.json b/composer.json index e95d2a4e..cf18b33c 100644 --- a/composer.json +++ b/composer.json @@ -28,6 +28,9 @@ "symfony/psr-http-message-bridge": "^1.0 | ^2.0", "nyholm/psr7": "^1.0" }, + "conflict": { + "laravel/lumen-framework": "*" + }, "autoload": { "psr-0": { "Sentry\\Laravel\\": "src/" diff --git a/src/Sentry/Laravel/Integration.php b/src/Sentry/Laravel/Integration.php index 905d3622..aabe8f15 100644 --- a/src/Sentry/Laravel/Integration.php +++ b/src/Sentry/Laravel/Integration.php @@ -167,68 +167,6 @@ public static function extractNameAndSourceForRoute(Route $route): array return [$routeName, $source]; } - /** - * Extract the readable name for a Lumen route. - * - * @param array $routeData The array of route data - * @param string $path The path of the request - * - * @return string - * - * @internal This helper is used in various places to extra meaninful info from a Lumen route data. - * @deprecated This will be removed in version 3.0, use `extractNameAndSourceForLumenRoute` instead. - */ - public static function extractNameForLumenRoute(array $routeData, string $path): string - { - return self::extractNameAndSourceForLumenRoute($routeData, $path)[0]; - } - - /** - * Extract the readable name for a Lumen route and the transaction source for where that route name came from. - * - * @param array $routeData The array of route data - * @param string $path The path of the request - * - * @return array{0: string, 1: \Sentry\Tracing\TransactionSource} - * - * @internal This helper is used in various places to extra meaninful info from a Lumen route data. - */ - public static function extractNameAndSourceForLumenRoute(array $routeData, string $path): array - { - $source = null; - $routeName = null; - - $route = $routeData[1] ?? []; - - // some.action (route name/alias) - if (!empty($route['as'])) { - $source = TransactionSource::component(); - $routeName = self::extractNameForNamedRoute($route['as']); - } - - // Some\Controller@someAction (controller action) - if (empty($routeName) && !empty($route['uses'])) { - $source = TransactionSource::component(); - $routeName = self::extractNameForActionRoute($route['uses']); - } - - // /some/{action} // Fallback to the route uri (with parameter placeholders) - if (empty($routeName) || $routeName === 'Closure') { - $routeUri = array_reduce( - array_keys($routeData[2]), - static function ($carry, $key) use ($routeData) { - return str_replace($routeData[2][$key], "{{$key}}", $carry); - }, - $path - ); - - $source = TransactionSource::url(); - $routeName = '/' . ltrim($routeUri, '/'); - } - - return [$routeName, $source]; - } - /** * Take a route name and return it only if it's a usable route name. * diff --git a/src/Sentry/Laravel/ServiceProvider.php b/src/Sentry/Laravel/ServiceProvider.php index 596c4cac..9edb5015 100644 --- a/src/Sentry/Laravel/ServiceProvider.php +++ b/src/Sentry/Laravel/ServiceProvider.php @@ -6,7 +6,6 @@ use Illuminate\Foundation\Application as Laravel; use Illuminate\Foundation\Http\Kernel as HttpKernel; use Illuminate\Log\LogManager; -use Laravel\Lumen\Application as Lumen; use RuntimeException; use Sentry\ClientBuilder; use Sentry\ClientBuilderInterface; @@ -48,10 +47,7 @@ public function boot(): void if ($this->hasDsnSet()) { $this->bindEvents(); - if ($this->app instanceof Lumen) { - $this->app->middleware(SetRequestMiddleware::class); - $this->app->middleware(SetRequestIpMiddleware::class); - } elseif ($this->app->bound(HttpKernelInterface::class)) { + if ($this->app->bound(HttpKernelInterface::class)) { /** @var \Illuminate\Foundation\Http\Kernel $httpKernel */ $httpKernel = $this->app->make(HttpKernelInterface::class); @@ -78,10 +74,6 @@ public function boot(): void */ public function register(): void { - if ($this->app instanceof Lumen) { - $this->app->configure(static::$abstract); - } - $this->mergeConfigFrom(__DIR__ . '/../../../config/sentry.php', static::$abstract); $this->configureAndRegisterClient(); diff --git a/src/Sentry/Laravel/Tracing/Middleware.php b/src/Sentry/Laravel/Tracing/Middleware.php index 589d1353..460ec941 100644 --- a/src/Sentry/Laravel/Tracing/Middleware.php +++ b/src/Sentry/Laravel/Tracing/Middleware.php @@ -91,9 +91,8 @@ public function terminate($request, $response): void * @param float|null $timestamp The unix timestamp of the booted event, default to `microtime(true)` if not `null`. * * @return void - * @internal This method should only be invoked right after the application has finished "booting": - * For Laravel this is from the application `booted` callback. - * For Lumen this is right before returning from the `bootstrap/app.php` file. + * + * @internal This method should only be invoked right after the application has finished "booting". */ public function setBootedTimestamp(?float $timestamp = null): void { @@ -190,18 +189,6 @@ private function hydrateRequestData(Request $request): void 'action' => $route->getActionName(), 'method' => $request->getMethod(), ]); - } elseif (is_array($route) && count($route) === 3) { - [$transactionName, $transactionSource] = Integration::extractNameAndSourceForLumenRoute($route, $request->path()); - - $this->updateTransactionNameIfDefault($transactionName, $transactionSource); - - $action = $route[1] ?? []; - - $this->transaction->setData([ - 'name' => $action['as'] ?? null, - 'action' => $action['uses'] ?? 'Closure', - 'method' => $request->getMethod(), - ]); } $this->updateTransactionNameIfDefault('/' . ltrim($request->path(), '/'), TransactionSource::url()); diff --git a/src/Sentry/Laravel/Tracing/ServiceProvider.php b/src/Sentry/Laravel/Tracing/ServiceProvider.php index 3235b3c4..336e6025 100644 --- a/src/Sentry/Laravel/Tracing/ServiceProvider.php +++ b/src/Sentry/Laravel/Tracing/ServiceProvider.php @@ -6,11 +6,9 @@ use Illuminate\Contracts\View\Engine; use Illuminate\Contracts\View\View; use Illuminate\Foundation\Http\Kernel as HttpKernel; -use Illuminate\Queue\QueueManager; use Illuminate\View\Engines\EngineResolver; use Illuminate\View\Factory as ViewFactory; use InvalidArgumentException; -use Laravel\Lumen\Application as Lumen; use Sentry\Laravel\BaseServiceProvider; use Sentry\Serializer\RepresentationSerializer; @@ -29,9 +27,7 @@ public function boot(): void $this->bindViewEngine($tracingConfig); - if ($this->app instanceof Lumen) { - $this->app->middleware(Middleware::class); - } elseif ($this->app->bound(HttpKernelInterface::class)) { + if ($this->app->bound(HttpKernelInterface::class)) { /** @var \Illuminate\Foundation\Http\Kernel $httpKernel */ $httpKernel = $this->app->make(HttpKernelInterface::class); @@ -55,11 +51,9 @@ public function register(): void return new BacktraceHelper($options, new RepresentationSerializer($options)); }); - if (!$this->app instanceof Lumen) { - $this->app->booted(function () { - $this->app->make(Middleware::class)->setBootedTimestamp(); - }); - } + $this->app->booted(function () { + $this->app->make(Middleware::class)->setBootedTimestamp(); + }); } private function bindEvents(array $tracingConfig): void diff --git a/test/Sentry/IntegrationTest.php b/test/Sentry/IntegrationTest.php index c116bfc2..34c98d27 100644 --- a/test/Sentry/IntegrationTest.php +++ b/test/Sentry/IntegrationTest.php @@ -162,83 +162,4 @@ public function testExtractingNameForRouteWithStrippedBaseNamespaceFromAction(): Integration::setControllersBaseNamespace(null); } - - public function testExtractingNameForLumenRouteWithName(): void - { - $route = [0, ['as' => $routeName = 'foo-bar'], []]; - - $this->assertSame($routeName, Integration::extractNameForLumenRoute($route, '/some-route')); - } - - public function testExtractingNameForLumenRouteWithAction(): void - { - $route = [0, ['uses' => $controller = 'SomeController@someAction'], []]; - - $this->assertSame($controller, Integration::extractNameForLumenRoute($route, '/some-route')); - } - - public function testExtractingNameForLumenRouteWithoutName(): void - { - $url = '/some-route'; - - $this->assertSame($url, Integration::extractNameForLumenRoute([0, [], []], $url)); - } - - public function testExtractingNameForLumenRouteWithParamInUrl(): void - { - $route = [1, [], ['param1' => 'foo']]; - - $url = '/foo/bar/baz'; - - $this->assertSame('/{param1}/bar/baz', Integration::extractNameForLumenRoute($route, $url)); - } - - public function testExtractingNameForLumenRouteWithParamsInUrl(): void - { - $route = [1, [], ['param1' => 'foo', 'param2' => 'bar']]; - - $url = '/foo/bar/baz'; - - $this->assertSame('/{param1}/{param2}/baz', Integration::extractNameForLumenRoute($route, $url)); - } - - public function testExtractingNameForLumenRouteWithActionAndName(): void - { - $route = [0, [ - 'as' => $routeName = 'foo-bar', - 'uses' => 'SomeController@someAction', - ], []]; - - $this->assertSame($routeName, Integration::extractNameForLumenRoute($route, '/some-route')); - } - - public function testExtractingNameForLumenRouteWithAutoGeneratedName(): void - { - // We fake a generated name here, Laravel generates them each starting with `generated::` - $route = [0, ['as' => 'generated::KoAePbpBofo01ey4'], []]; - - $url = '/some-route'; - - $this->assertSame($url, Integration::extractNameForLumenRoute($route, $url)); - } - - public function testExtractingNameForLumenRouteWithIncompleteGroupName(): void - { - $route = [0, ['as' => 'group-name.'], []]; - - $url = '/some-route'; - - $this->assertSame($url, Integration::extractNameForLumenRoute($route, $url)); - } - - public function testExtractingNameForLumenRouteWithStrippedBaseNamespaceFromAction(): void - { - Integration::setControllersBaseNamespace('BaseNamespace'); - - $route = [0, ['uses' => 'BaseNamespace\\SomeController@someAction'], []]; - - $this->assertSame('SomeController@someAction', Integration::extractNameForLumenRoute($route, '/some-route')); - - Integration::setControllersBaseNamespace(null); - } } From 42d5cb872343a225878a188093aa8881dea1f6d4 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 6 Oct 2022 11:20:56 +0200 Subject: [PATCH 2/9] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c09af0ed..074b1fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Drop support for Laravel Lumen (#579) + ## 2.14.0 - Fix not listening to queue events because `QueueManager` is registered as `queue` in the container and not by it's class name (#568) From 15745dee5e14a176bddad192b4107a6e042d6cf0 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 6 Oct 2022 11:26:17 +0200 Subject: [PATCH 3/9] Set the tracing transaction name when a route matches --- src/Sentry/Laravel/Integration.php | 13 ++++ src/Sentry/Laravel/Tracing/EventHandler.php | 55 ++++++++++++++-- src/Sentry/Laravel/Tracing/Middleware.php | 70 +++++---------------- 3 files changed, 78 insertions(+), 60 deletions(-) diff --git a/src/Sentry/Laravel/Integration.php b/src/Sentry/Laravel/Integration.php index aabe8f15..6c7921cb 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; @@ -252,6 +253,18 @@ public static function sentryBaggageMeta(): string return $content; } + /** + * 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/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index 1fafb3bd..3364525c 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -10,6 +10,8 @@ use Illuminate\Queue\Events as QueueEvents; use Illuminate\Queue\Queue; use Illuminate\Queue\QueueManager; +use Illuminate\Routing\Events as RoutingEvents; +use Illuminate\Routing\Route; use RuntimeException; use Sentry\Laravel\Integration; use Sentry\SentrySdk; @@ -30,6 +32,9 @@ class EventHandler * @var array */ protected static $eventHandlerMap = [ + 'router.matched' => 'routerMatched', // Until Laravel 5.1 + RoutingEvents\RouteMatched::class => 'routeMatched', // Since Laravel 5.2 + 'illuminate.query' => 'query', // Until Laravel 5.1 DatabaseEvents\QueryExecuted::class => 'queryExecuted', // Since Laravel 5.2 ]; @@ -122,6 +127,11 @@ public function __construct(Container $container, BacktraceHelper $backtraceHelp /** * Attach all event handlers. + * + * @uses self::routeMatchedHandler() + * @uses self::routerMatchedHandler() + * @uses self::queryHandler() + * @uses self::queryExecutedHandler() */ public function subscribe(): void { @@ -141,6 +151,10 @@ public function subscribe(): void * Attach all queue event handlers. * * @param \Illuminate\Queue\QueueManager $queue + * + * @uses self::queueJobProcessedHandler() + * @uses self::queueJobProcessingHandler() + * @uses self::queueJobExceptionOccurredHandler() */ public function subscribeQueueEvents(QueueManager $queue): void { @@ -185,7 +199,7 @@ public function subscribeQueueEvents(QueueManager $queue): void * @param string $method * @param array $arguments */ - public function __call($method, $arguments) + public function __call(string $method, array $arguments) { $handlerMethod = "{$method}Handler"; @@ -200,6 +214,35 @@ public function __call($method, $arguments) } } + /** + * Until Laravel 5.1 + * + * @param Route $route + */ + protected function routerMatchedHandler(Route $route): void + { + $transaction = Integration::currentTransaction(); + + if ($transaction === null) { + return; + } + + [$transactionName, $transactionSource] = Integration::extractNameAndSourceForRoute($route); + + $transaction->setName($transactionName); + $transaction->getMetadata()->setSource($transactionSource); + } + + /** + * Since Laravel 5.2 + * + * @param \Illuminate\Routing\Events\RouteMatched $match + */ + protected function routeMatchedHandler(RoutingEvents\RouteMatched $match): void + { + $this->routerMatchedHandler($match->route); + } + /** * Until Laravel 5.1 * @@ -249,7 +292,7 @@ private function recordQuerySpan($query, $time): void $context->setEndTimestamp($context->getStartTimestamp() + $time / 1000); if ($this->traceSqlQueryOrigins) { - $queryOrigin = $this->resolveQueryOriginFromBacktrace($context); + $queryOrigin = $this->resolveQueryOriginFromBacktrace(); if ($queryOrigin !== null) { $context->setData(['sql.origin' => $queryOrigin]); @@ -277,12 +320,12 @@ private function resolveQueryOriginFromBacktrace(): ?string return "{$filePath}:{$firstAppFrame->getLine()}"; } - /* + /** * Since Laravel 5.2 * * @param \Illuminate\Queue\Events\JobProcessing $event */ - protected function queueJobProcessingHandler(QueueEvents\JobProcessing $event) + protected function queueJobProcessingHandler(QueueEvents\JobProcessing $event): void { $parentSpan = Integration::currentTracingSpan(); @@ -352,7 +395,7 @@ protected function queueJobProcessingHandler(QueueEvents\JobProcessing $event) * * @param \Illuminate\Queue\Events\JobExceptionOccurred $event */ - protected function queueJobExceptionOccurredHandler(QueueEvents\JobExceptionOccurred $event) + protected function queueJobExceptionOccurredHandler(QueueEvents\JobExceptionOccurred $event): void { $this->afterQueuedJob(SpanStatus::internalError()); } @@ -362,7 +405,7 @@ protected function queueJobExceptionOccurredHandler(QueueEvents\JobExceptionOccu * * @param \Illuminate\Queue\Events\JobProcessed $event */ - protected function queueJobProcessedHandler(QueueEvents\JobProcessed $event) + protected function queueJobProcessedHandler(QueueEvents\JobProcessed $event): void { $this->afterQueuedJob(SpanStatus::ok()); } diff --git a/src/Sentry/Laravel/Tracing/Middleware.php b/src/Sentry/Laravel/Tracing/Middleware.php index 460ec941..89f2f35c 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,12 +55,12 @@ 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)) { if ($this->appSpan !== null) { @@ -73,11 +71,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 +102,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 +121,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 +142,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 +174,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()); - } } From 0bb8b215eebb0ab9922eba80a2047e49609a09ad Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 6 Oct 2022 11:33:10 +0200 Subject: [PATCH 4/9] Update tests and drop deprecated method --- src/Sentry/Laravel/Integration.php | 15 --------------- test/Sentry/IntegrationTest.php | 23 ++++++++++++++++------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/Sentry/Laravel/Integration.php b/src/Sentry/Laravel/Integration.php index 6c7921cb..60034932 100644 --- a/src/Sentry/Laravel/Integration.php +++ b/src/Sentry/Laravel/Integration.php @@ -118,21 +118,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. * diff --git a/test/Sentry/IntegrationTest.php b/test/Sentry/IntegrationTest.php index 34c98d27..400ec74d 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 @@ -107,7 +108,7 @@ public function testExtractingNameForRouteWithName(): void { $route = (new Route('GET', '/foo', []))->name($routeName = 'foo-bar'); - $this->assertSame($routeName, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $routeName, TransactionSource::component()); } public function testExtractingNameForRouteWithAction(): void @@ -116,14 +117,14 @@ public function testExtractingNameForRouteWithAction(): void 'controller' => $controller = 'SomeController@someAction', ])); - $this->assertSame($controller, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $controller, TransactionSource::component()); } public function testExtractingNameForRouteWithoutName(): void { $route = new Route('GET', $url = '/foo', []); - $this->assertSame($url, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } public function testExtractingNameForRouteWithActionAndName(): void @@ -132,7 +133,7 @@ public function testExtractingNameForRouteWithActionAndName(): void 'controller' => 'SomeController@someAction', ]))->name($routeName = 'foo-bar'); - $this->assertSame($routeName, Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, $routeName, TransactionSource::component()); } public function testExtractingNameForRouteWithAutoGeneratedName(): void @@ -140,14 +141,14 @@ 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 @@ -158,8 +159,16 @@ public function testExtractingNameForRouteWithStrippedBaseNamespaceFromAction(): 'controller' => 'BaseNamespace\\SomeController@someAction', ])); - $this->assertSame('SomeController@someAction', Integration::extractNameForRoute($route)); + $this->assetRouteNameAndSource($route, 'SomeController@someAction', TransactionSource::component()); Integration::setControllersBaseNamespace(null); } + + private function assetRouteNameAndSource(Route $route, string $expectedName, TransactionSource $expectedSource): void + { + [$actualName, $actualSource] = Integration::extractNameAndSourceForRoute($route); + + $this->assertSame($expectedName, $actualName); + $this->assertSame($expectedSource, $actualSource); + } } From 2dbbc785830b256ad29d85677a599ede5a017f96 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 6 Oct 2022 11:48:17 +0200 Subject: [PATCH 5/9] Add changelog entries --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 074b1fe0..96403ce1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## 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) ## 2.14.0 From 5d037090d5857bccaaa437e402b820057256a227 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Fri, 7 Oct 2022 15:03:36 +0200 Subject: [PATCH 6/9] =?UTF-8?q?Don=E2=80=99t=20trace=20missing=20routes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- config/sentry.php | 3 +++ src/Sentry/Laravel/Tracing/Middleware.php | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/config/sentry.php b/config/sentry.php index b0e4b700..e8fd84c8 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 diff --git a/src/Sentry/Laravel/Tracing/Middleware.php b/src/Sentry/Laravel/Tracing/Middleware.php index 89f2f35c..33d2afe2 100644 --- a/src/Sentry/Laravel/Tracing/Middleware.php +++ b/src/Sentry/Laravel/Tracing/Middleware.php @@ -63,6 +63,11 @@ 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 ($this->appSpan !== null) { $this->appSpan->finish(); } From 8614807c4a84336d8f6cbe83d2797c21746edc7c Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Fri, 7 Oct 2022 15:21:22 +0200 Subject: [PATCH 7/9] Little simplification on queue job transaction context --- src/Sentry/Laravel/Tracing/EventHandler.php | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index 87dbc879..9513c465 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -295,24 +295,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()); } From 78817c5a4176ee376873934a79bf54b45ee006b0 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Tue, 11 Oct 2022 14:11:33 +0200 Subject: [PATCH 8/9] Remove extracting route name or controller for transaction names (#583) * Remove extracting route name or controller for transaction names * CS * Add changelog entry --- CHANGELOG.md | 1 + config/sentry.php | 2 - src/Sentry/Laravel/Integration.php | 94 ++------------------------ src/Sentry/Laravel/ServiceProvider.php | 13 ++-- test/Sentry/IntegrationTest.php | 38 ----------- 5 files changed, 11 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e72b863..09e1e30e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Remove `Sentry\Integration::extractNameForRoute()`, it's alternative `Sentry\Integration::extractNameAndSourceForRoute()` is marked as `@internal` (#580) - Drop support for Laravel 5.x (#581) - 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 e8fd84c8..08c918bf 100644 --- a/config/sentry.php +++ b/config/sentry.php @@ -55,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 11a2ec08..773977c9 100644 --- a/src/Sentry/Laravel/Integration.php +++ b/src/Sentry/Laravel/Integration.php @@ -22,11 +22,6 @@ class Integration implements IntegrationInterface */ private static $transaction; - /** - * @var null|string - */ - private static $baseControllerNamespace; - /** * {@inheritdoc} */ @@ -95,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. * @@ -129,75 +116,10 @@ public static function flushEvents(): void */ 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() + ]; } /** @@ -213,9 +135,7 @@ public static function sentryTracingMeta(): string return ''; } - $content = sprintf('', $span->toTraceparent()); - - return $content; + return sprintf('', $span->toTraceparent()); } /** @@ -232,9 +152,7 @@ public static function sentryBaggageMeta(): string return ''; } - $content = sprintf('', $span->toBaggage()); - - return $content; + return sprintf('', $span->toBaggage()); } /** 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/test/Sentry/IntegrationTest.php b/test/Sentry/IntegrationTest.php index 4434eca0..a2ed7af2 100644 --- a/test/Sentry/IntegrationTest.php +++ b/test/Sentry/IntegrationTest.php @@ -89,22 +89,6 @@ public function testTransactionIsNotAppliedToEventWhenTransactionIsAlreadySet(): }); } - public function testExtractingNameForRouteWithName(): void - { - $route = (new Route('GET', '/foo', []))->name($routeName = 'foo-bar'); - - $this->assetRouteNameAndSource($route, $routeName, TransactionSource::component()); - } - - public function testExtractingNameForRouteWithAction(): void - { - $route = (new Route('GET', '/foo', [ - 'controller' => $controller = 'SomeController@someAction', - ])); - - $this->assetRouteNameAndSource($route, $controller, TransactionSource::component()); - } - public function testExtractingNameForRouteWithoutName(): void { $route = new Route('GET', $url = '/foo', []); @@ -112,15 +96,6 @@ public function testExtractingNameForRouteWithoutName(): void $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } - public function testExtractingNameForRouteWithActionAndName(): void - { - $route = (new Route('GET', '/foo', [ - 'controller' => 'SomeController@someAction', - ]))->name($routeName = 'foo-bar'); - - $this->assetRouteNameAndSource($route, $routeName, TransactionSource::component()); - } - public function testExtractingNameForRouteWithAutoGeneratedName(): void { // We fake a generated name here, Laravel generates them each starting with `generated::` @@ -136,19 +111,6 @@ public function testExtractingNameForRouteWithIncompleteGroupName(): void $this->assetRouteNameAndSource($route, $url, TransactionSource::route()); } - public function testExtractingNameForRouteWithStrippedBaseNamespaceFromAction(): void - { - Integration::setControllersBaseNamespace('BaseNamespace'); - - $route = (new Route('GET', '/foo', [ - 'controller' => 'BaseNamespace\\SomeController@someAction', - ])); - - $this->assetRouteNameAndSource($route, 'SomeController@someAction', TransactionSource::component()); - - Integration::setControllersBaseNamespace(null); - } - private function assetRouteNameAndSource(Route $route, string $expectedName, TransactionSource $expectedSource): void { [$actualName, $actualSource] = Integration::extractNameAndSourceForRoute($route); From e15abc6624eb8da1c7c6f7f22ed40315c039f446 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Wed, 12 Oct 2022 11:07:54 +0200 Subject: [PATCH 9/9] Add span for Laravel HTTP client requests (#585) --- src/Sentry/Laravel/Tracing/EventHandler.php | 68 +++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php index 9513c465..d971d32a 100644 --- a/src/Sentry/Laravel/Tracing/EventHandler.php +++ b/src/Sentry/Laravel/Tracing/EventHandler.php @@ -7,6 +7,7 @@ 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; @@ -33,6 +34,9 @@ class EventHandler protected static $eventHandlerMap = [ RoutingEvents\RouteMatched::class => 'routeMatched', DatabaseEvents\QueryExecuted::class => 'queryExecuted', + HttpClientEvents\RequestSending::class => 'httpClientRequestSending', + HttpClientEvents\ResponseReceived::class => 'httpClientResponseReceived', + HttpClientEvents\ConnectionFailed::class => 'httpClientConnectionFailed', ]; /** @@ -95,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. * @@ -267,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();