Remove extracting route name or controller for transaction names (#583)

* Remove extracting route name or controller for transaction names * CS * Add changelog entry
getsentry · Oct 11, 2022 · 78817c5 · 78817c5
1 parent 8614807
commit 78817c5
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 137 deletions.
diff --git 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
@@ -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
@@ -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('<meta name="sentry-trace" content="%s"/>', $span->toTraceparent());
-
-        return $content;
+        return sprintf('<meta name="sentry-trace" content="%s"/>', $span->toTraceparent());
     }
 
     /**
@@ -232,9 +152,7 @@ public static function sentryBaggageMeta(): string
             return '';
         }
 
-        $content = sprintf('<meta name="baggage" content="%s"/>', $span->toBaggage());
-
-        return $content;
+        return sprintf('<meta name="baggage" content="%s"/>', $span->toBaggage());
     }
 
     /**

diff --git 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,15 +156,15 @@ 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);
 
             $options = $clientBuilder->getOptions();
 
             $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
@@ -89,38 +89,13 @@ 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', []);
 
         $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);