Skip to content

Commit 5404b05

Browse files
cataphractcataphract
authored
Merge pull request #3676 from DataDog/glopes/symfony-routes-redux
Optimize Symfony http.route caching with path map approach
2 parents 2c440ab + 756bbd7 commit 5404b05

2 files changed

Lines changed: 127 additions & 56 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,14 +125,17 @@ class Symfony62Tests {
125125
service apache2 restart''')
126126
assert res.exitCode == 0
127127

128+
// path params are always pushed to AppSec regardless of DD_TRACE_SYMFONY_HTTP_ROUTE,
129+
// so the WAF still blocks based on the path param key 'param01'
128130
HttpRequest req = container.buildReq('/dynamic-path/someValue').GET().build()
129131
def trace = container.traceFromRequest(req, ofString()) { HttpResponse<String> re ->
130-
assert re.statusCode() == 200
131-
assert re.body().contains('Hi someValue!')
132+
assert re.statusCode() == 403
132133
}
133134

134135
Span span = trace.first()
135-
assert span.meta."http.route" != '/dynamic-path/{param01}'
136+
assert span.meta."http.route" == null
137+
assert span.meta."symfony.route.name" != null
138+
assert span.resource == 'app_home_dynamic'
136139
} finally {
137140
def res = CONTAINER.execInContainer(
138141
'bash', '-c',

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

Lines changed: 121 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -422,89 +422,157 @@ static function() {
422422
);
423423

424424
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+
*/
425435
$handle_http_route = static function($route_name, $request, $rootSpan) {
426436
if (self::$kernel === null) {
427437
return;
428438
}
439+
429440
/** @var ContainerInterface $container */
430441
$container = self::$kernel->getContainer();
442+
431443
try {
432444
$cache = $container->get('cache.app');
433445
} catch (\Exception $e) {
434446
return;
435447
}
436448

437-
/** @var \Symfony\Bundle\FrameworkBundle\Routing\Router $router */
438-
$router = $container->get('router');
439449
if (!\method_exists($cache, 'getItem')) {
440450
return;
441451
}
442-
$itemName = "_datadog.route.path.$route_name";
443-
$locale = $request->get('_locale');
444-
if ($locale !== null) {
445-
$itemName .= ".$locale";
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+
}
446468
}
447-
$item = $cache->getItem($itemName);
448-
if ($item->isHit()) {
449-
$route = $item->get();
450-
} else {
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 = [];
451496
$routeCollection = $router->getRouteCollection();
452-
$route = $routeCollection->get($route_name);
453-
if ($route == null && $locale !== null) {
454-
$route = $routeCollection->get($route_name . '.' . $locale);
497+
foreach ($routeCollection->all() as $name => $route) {
498+
$routePathMap[$name] = $route->getPath();
455499
}
456-
$item->set($route);
457-
$item->expiresAfter(3600);
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+
]);
458511
$cache->save($item);
459512
}
460-
if (isset($route)) {
461-
$rootSpan->meta[Tag::HTTP_ROUTE] = $route->getPath();
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)
520+
$locale = $request->get('_locale');
521+
if ($locale !== null && isset($routePathMap[$route_name . '.' . $locale])) {
522+
$path = $routePathMap[$route_name . '.' . $locale];
523+
}
524+
}
525+
526+
if ($path !== null) {
527+
$rootSpan->meta[Tag::HTTP_ROUTE] = $path;
462528
}
463529
};
530+
} else {
531+
$handle_http_route = static function() { /* noop */ };
532+
}
464533

465-
\DDTrace\trace_method(
466-
'Symfony\Component\HttpKernel\HttpKernel',
467-
'handle',
468-
static function(SpanData $span, $args, $response) use ($handle_http_route) {
469-
/** @var Request $request */
470-
list($request) = $args;
534+
\DDTrace\trace_method(
535+
'Symfony\Component\HttpKernel\HttpKernel',
536+
'handle',
537+
static function(SpanData $span, $args, $response) use ($handle_http_route) {
538+
/** @var Request $request */
539+
list($request) = $args;
471540

472-
$span->name = 'symfony.kernel.handle';
473-
$span->service = \ddtrace_config_app_name(self::$frameworkPrefix);
474-
$span->type = Type::WEB_SERVLET;
475-
$span->meta[Tag::COMPONENT] = self::NAME;
541+
$span->name = 'symfony.kernel.handle';
542+
$span->service = \ddtrace_config_app_name(self::$frameworkPrefix);
543+
$span->type = Type::WEB_SERVLET;
544+
$span->meta[Tag::COMPONENT] = self::NAME;
476545

477-
$rootSpan = \DDTrace\root_span();
478-
$rootSpan->meta[Tag::HTTP_METHOD] = $request->getMethod();
479-
$rootSpan->meta[Tag::COMPONENT] = self::$frameworkPrefix;
480-
$rootSpan->meta[Tag::SPAN_KIND] = 'server';
481-
self::addTraceAnalyticsIfEnabled($rootSpan);
546+
$rootSpan = \DDTrace\root_span();
547+
$rootSpan->meta[Tag::HTTP_METHOD] = $request->getMethod();
548+
$rootSpan->meta[Tag::COMPONENT] = self::$frameworkPrefix;
549+
$rootSpan->meta[Tag::SPAN_KIND] = 'server';
550+
self::addTraceAnalyticsIfEnabled($rootSpan);
482551

483-
if (!array_key_exists(Tag::HTTP_URL, $rootSpan->meta)) {
484-
$rootSpan->meta[Tag::HTTP_URL] = Normalizer::urlSanitize($request->getUri());
485-
}
486-
if (isset($response)) {
487-
$rootSpan->meta[Tag::HTTP_STATUS_CODE] = $response->getStatusCode();
488-
}
552+
if (!array_key_exists(Tag::HTTP_URL, $rootSpan->meta)) {
553+
$rootSpan->meta[Tag::HTTP_URL] = Normalizer::urlSanitize($request->getUri());
554+
}
555+
if (isset($response)) {
556+
$rootSpan->meta[Tag::HTTP_STATUS_CODE] = $response->getStatusCode();
557+
}
489558

490-
$route_name = $request->get('_route');
491-
if ($route_name !== null) {
492-
if (dd_trace_env_config("DD_HTTP_SERVER_ROUTE_BASED_NAMING")) {
493-
$rootSpan->resource = $route_name;
494-
}
495-
$rootSpan->meta['symfony.route.name'] = $route_name;
496-
$handle_http_route($route_name, $request, $rootSpan);
559+
$route_name = $request->get('_route');
560+
if ($route_name !== null) {
561+
if (dd_trace_env_config("DD_HTTP_SERVER_ROUTE_BASED_NAMING")) {
562+
$rootSpan->resource = $route_name;
497563
}
564+
$rootSpan->meta['symfony.route.name'] = $route_name;
565+
$handle_http_route($route_name, $request, $rootSpan);
566+
}
498567

499-
$parameters = $request->get('_route_params');
500-
if (!empty($parameters) &&
501-
is_array($parameters) &&
502-
function_exists('datadog\appsec\push_addresses')) {
503-
\datadog\appsec\push_addresses(["server.request.path_params" => $parameters]);
504-
}
568+
$parameters = $request->get('_route_params');
569+
if (!empty($parameters) &&
570+
is_array($parameters) &&
571+
function_exists('datadog\appsec\push_addresses')) {
572+
\datadog\appsec\push_addresses(["server.request.path_params" => $parameters]);
505573
}
506-
);
507-
}
574+
}
575+
);
508576

509577
/*
510578
* EventDispatcher v4.3 introduced an arg hack that mutates the arguments.

0 commit comments

Comments
 (0)