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

Remove extracting route name or controller for transaction names #583

Merged
merged 3 commits into from Oct 11, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -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

Expand Down
2 changes: 0 additions & 2 deletions config/sentry.php
Expand Up @@ -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'),

];
94 changes: 6 additions & 88 deletions src/Sentry/Laravel/Integration.php
Expand Up @@ -22,11 +22,6 @@ class Integration implements IntegrationInterface
*/
private static $transaction;

/**
* @var null|string
*/
private static $baseControllerNamespace;

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -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.
*
Expand All @@ -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()
];
}

/**
Expand All @@ -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());
}

/**
Expand All @@ -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());
}

/**
Expand Down
13 changes: 4 additions & 9 deletions src/Sentry/Laravel/ServiceProvider.php
Expand Up @@ -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',
];

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
38 changes: 0 additions & 38 deletions test/Sentry/IntegrationTest.php
Expand Up @@ -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::`
Expand All @@ -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);
Expand Down