Skip to content

Commit 8eb1101

Browse files
cataphractcataphract
committed
Again improve symfony http.route resolution
Turns out it's quite feasible to resolve at runtime the route path from the route name using files in the Symfony cache dir. Not only that, but that strategy was already being used in EndpointCatalog.php. This fixes a performance issue -- avoid the deserializaiton and possible transfer from Redis of the full route collection cache on every request. The routing data is in PHP files and with opcache it's basically just one big compile-time variable.
1 parent 14dabf8 commit 8eb1101

4 files changed

Lines changed: 73 additions & 90 deletions

File tree

‎appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Symfony62Tests.groovy‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,31 @@ class Symfony62Tests {
117117

118118
@Test
119119
@Order(8)
120+
void 'http route for locale route'() {
121+
HttpRequest req = container.buildReq('/caminho-dinamico/someValue').GET().build()
122+
def trace = container.traceFromRequest(req, ofString()) { HttpResponse<String> re ->
123+
assert re.statusCode() == 403
124+
assert re.body().contains('blocked')
125+
}
126+
127+
Span span = trace.first()
128+
assert span.meta."http.route" == '/caminho-dinamico/{param01}'
129+
}
130+
131+
@Test
132+
@Order(9)
133+
void 'http route for utf8 route'() {
134+
HttpRequest req = container.buildReq('/café/espresso').GET().build()
135+
def trace = container.traceFromRequest(req, ofString()) { HttpResponse<String> re ->
136+
assert re.statusCode() == 200
137+
}
138+
139+
Span span = trace.first()
140+
assert span.meta."http.route" == '/café/{item}'
141+
}
142+
143+
@Test
144+
@Order(10)
120145
void 'symfony http route disabled'() {
121146
try {
122147
def res = CONTAINER.execInContainer(

‎appsec/tests/integration/src/test/www/symfony62/src/Controller/HomeController.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,4 +29,12 @@ public function dynamicAction(Request $request, string $param01)
2929
"Hi $param01!"
3030
);
3131
}
32+
33+
#[Route("/café/{item}", name: "utf8_route")]
34+
public function utf8Action(Request $request, string $item)
35+
{
36+
return new Response(
37+
"Café: $item"
38+
);
39+
}
3240
}

‎src/DDTrace/Integrations/Symfony/EndpointCatalog.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,41 @@ public static function generate(ContainerInterface $container): array
2929
return $memo = $fromCollection;
3030
}
3131

32+
33+
/**
34+
* Looks up the path template for a given route name from the compiled
35+
* url_generating_routes.php file. Returns null if the file is missing
36+
* or the route is not found.
37+
*
38+
* This never falls back to getRouteCollection().
39+
*/
40+
public static function pathForRoute($routeName, ContainerInterface $container)
41+
{
42+
$cacheDir = self::getCacheDir($container);
43+
if ($cacheDir === null) {
44+
return null;
45+
}
46+
47+
$genFile = rtrim($cacheDir, '/\\') . DIRECTORY_SEPARATOR . 'url_generating_routes.php';
48+
if (!is_file($genFile)) {
49+
return null;
50+
}
51+
52+
$gen = require $genFile;
53+
if (!is_array($gen) || !isset($gen[$routeName])) {
54+
return null;
55+
}
56+
57+
$routeData = $gen[$routeName];
58+
$tokens = isset($routeData[3]) ? $routeData[3] : null;
59+
if (!is_array($tokens)) {
60+
return null;
61+
}
62+
63+
return self::tokensToPathTemplate($tokens);
64+
}
65+
66+
3267
private static function getCacheDir(ContainerInterface $container)
3368
{
3469
if ($container->hasParameter('kernel.cache_dir')) {

‎src/DDTrace/Integrations/Symfony/SymfonyIntegration.php‎

Lines changed: 5 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
use Symfony\Component\DependencyInjection\ContainerInterface;
1313
use Symfony\Component\HttpFoundation\Request;
1414
use Symfony\Component\HttpKernel\KernelEvents;
15-
use Symfony\Contracts\Cache\ItemInterface;
1615

1716
class SymfonyIntegration extends Integration
1817
{
@@ -422,104 +421,20 @@ static function() {
422421
);
423422

424423
if (\dd_trace_env_config('DD_TRACE_SYMFONY_HTTP_ROUTE')) {
425-
/**
426-
* Resolves the http.route tag for a given route name by looking up
427-
* the route path in a cached map of all routes.
428-
*
429-
* Caching strategy:
430-
* - Caches the entire route path map under a single key: '_datadog.symfony.route_paths'
431-
* - Stores: ['mtime' => timestamp, 'paths' => ['route_name' => '/path', ...]]
432-
* - Invalidates cache when Symfony's compiled routes file is newer than cached mtime
433-
* - Falls back gracefully if cache.app is unavailable (no http.route tag)
434-
*/
435424
$handle_http_route = static function($route_name, $request, $rootSpan) {
436425
if (self::$kernel === null) {
437426
return;
438427
}
439428

440429
/** @var ContainerInterface $container */
441430
$container = self::$kernel->getContainer();
431+
$path = EndpointCatalog::pathForRoute($route_name, $container);
442432

443-
try {
444-
$cache = $container->get('cache.app');
445-
} catch (\Exception $e) {
446-
return;
447-
}
448-
449-
if (!\method_exists($cache, 'getItem')) {
450-
return;
451-
}
452-
453-
/** @var \Symfony\Bundle\FrameworkBundle\Routing\Router $router */
454-
try {
455-
$router = $container->get('router');
456-
} catch (\Exception $e) {
457-
return;
458-
}
459-
460-
// Get the compiled routes file mtime for cache invalidation
461-
$compiledRoutesMtime = null;
462-
$cacheDir = \method_exists($router, 'getOption') ? $router->getOption('cache_dir') : null;
463-
if ($cacheDir !== null) {
464-
$compiledRoutesFile = $cacheDir . '/url_generating_routes.php';
465-
if (\file_exists($compiledRoutesFile)) {
466-
$compiledRoutesMtime = @\filemtime($compiledRoutesFile);
467-
}
468-
}
469-
470-
$cacheKey = '_datadog.symfony.route_paths';
471-
/** @var ItemInterface $item */
472-
$item = $cache->getItem($cacheKey);
473-
$cachedData = $item->isHit() ? $item->get() : null;
474-
475-
$routePathMap = null;
476-
$needsRebuild = true;
477-
478-
if (\is_array($cachedData) && isset($cachedData['paths']) && \is_array($cachedData['paths'])) {
479-
// Check if cache is still valid
480-
if ($compiledRoutesMtime === null) {
481-
// No compiled file to check against - cache is valid
482-
$needsRebuild = false;
483-
$routePathMap = $cachedData['paths'];
484-
} elseif (isset($cachedData['mtime']) && $cachedData['mtime'] >= $compiledRoutesMtime) {
485-
// Cached data is newer than or equal to compiled routes - cache is valid
486-
$needsRebuild = false;
487-
$routePathMap = $cachedData['paths'];
488-
}
489-
// Otherwise: compiled routes file is newer, rebuild cache
490-
}
491-
492-
if ($needsRebuild) {
493-
$startTime = \function_exists('hrtime') ? \hrtime(true) : null;
494-
495-
$routePathMap = [];
496-
$routeCollection = $router->getRouteCollection();
497-
foreach ($routeCollection->all() as $name => $route) {
498-
$routePathMap[$name] = $route->getPath();
499-
}
500-
501-
if ($startTime !== null) {
502-
$durationNanoseconds = \hrtime(true) - $startTime;
503-
$durationMicroseconds = (int)($durationNanoseconds / 1000);
504-
$rootSpan->metrics['_dd.symfony.route.map_build_duration_us'] = $durationMicroseconds;
505-
}
506-
507-
$item->set([
508-
'mtime' => \time(),
509-
'paths' => $routePathMap,
510-
]);
511-
$cache->save($item);
512-
}
513-
514-
// Look up the route path
515-
$path = null;
516-
if (isset($routePathMap[$route_name])) {
517-
$path = $routePathMap[$route_name];
518-
} else {
519-
// Try with locale suffix (Symfony i18n routing convention)
433+
// Try with locale suffix (Symfony i18n routing convention)
434+
if ($path === null) {
520435
$locale = $request->get('_locale');
521-
if ($locale !== null && isset($routePathMap[$route_name . '.' . $locale])) {
522-
$path = $routePathMap[$route_name . '.' . $locale];
436+
if ($locale !== null) {
437+
$path = EndpointCatalog::pathForRoute($route_name . '.' . $locale, $container);
523438
}
524439
}
525440

0 commit comments

Comments
 (0)