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

Add http.route tag to SymfonyIntegration.php #2992

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,6 @@ TEST_WEB_81 := \
test_web_nette_30 \
test_web_slim_312 \
test_web_slim_4 \
test_web_symfony_52 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing those tests from PHP 8.1 up to PHP 8.3? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symfony 5.2 doesn't actually work with those PHP versions. The tests fail with:

[17-Dec-2024 19:40:57 -03] [ddtrace] [warning] Error raised in ddtrace's closure defined at /Users/gustavo.lopes/repos/dd-trace-php/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php:394 for Symfony\Component\HttpKernel\HttpKernel::handle(): Return type of Symfony\Component\Routing\RouteCollection::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /Users/gustavo.lopes/repos/dd-trace-php/tests/Frameworks/Symfony/Version_5_2/vendor/symfony/routing/RouteCollection.php on line 69
 /Users/gustavo.lopes/repos/dd-trace-php/tests/Common/WebFrameworkTestCase.php:74
 /Users/gustavo.lopes/repos/dd-trace-php/tests/Common/MultiPHPUnitVersionAdapter_typed.php:33

test_web_wordpress_59 \
test_web_wordpress_61 \
test_web_custom \
Expand Down Expand Up @@ -981,7 +980,6 @@ TEST_WEB_82 := \
test_web_nette_30 \
test_web_slim_312 \
test_web_slim_4 \
test_web_symfony_52 \
test_web_symfony_62 \
test_web_symfony_70 \
test_web_wordpress_59 \
Expand Down Expand Up @@ -1040,7 +1038,6 @@ TEST_WEB_83 := \
test_web_nette_30 \
test_web_slim_312 \
test_web_slim_4 \
test_web_symfony_52 \
test_web_symfony_62 \
test_web_symfony_70 \
test_web_wordpress_59 \
Expand Down
1 change: 1 addition & 0 deletions appsec/tests/integration/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ task loadCaches(type: Exec) {

commandLine 'docker', 'run', '--rm',
'-v', 'php-tracer-cargo-cache:/caches/php-tracer-cargo-cache',
'-v', 'php-tracer-cargo-cache-git:/caches/php-tracer-cargo-cache-git',
'-v', 'php-appsec-hunter-cache:/caches/php-appsec-hunter-cache',
'-v', "${project.buildDir}:/build",
'busybox',
Expand Down
1 change: 1 addition & 0 deletions appsec/tests/integration/gradle/images.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ task saveCaches(type: Exec) {
commandLine 'docker', 'run', '--rm',
'-e', "UUID=${uuid}",
'-v', 'php-tracer-cargo-cache:/caches/php-tracer-cargo-cache',
'-v', 'php-tracer-cargo-cache-git:/caches/php-tracer-cargo-cache-git',
'-v', 'php-appsec-hunter-cache:/caches/php-appsec-hunter-cache',
'-v', "${project.buildDir}:/build",
'busybox',
Expand Down
35 changes: 28 additions & 7 deletions appsec/tests/integration/src/docker/php/build_dev_php.sh
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,36 @@ EOD
chmod +x /tmp/apxs_wrapper
}

function run_dsymutil {
if [[ $(uname) != Darwin ]] then
return
fi
local readonly dir=$1 exe=
find "$dir" -type f -exec test -x '{}' \; -print | while read -r exe; do
if [[ $exe != *.a ]]; then
if ! grep -q '^#!' "$exe"; then
local readonly dSYM_DIR="${exe}.dSYM"
if [[ ! -d $dSYM_DIR ]]; then
dsymutil "$exe"
fi
fi
fi
done
}

function get_xdebug_version {
local -r version=$1
local readonly version_id=$(php_version_id $version)
if [[ $version_id -lt 70300 ]]; then
echo '2.8.1'
elif [[ $version_id -lt 80000 ]]; then
if [[ $version_id -lt 70100 ]]; then
echo '2.6.1'
elif [[ $version_id -lt 70200 ]]; then
echo '2.9.8'
elif [[ $version_id -lt 80000 ]]; then
echo '3.1.6'
elif [[ $version_id -lt 80400 ]]; then
echo '3.3.1'
echo '3.3.2'
elif [[ $version_id -ge 80400 ]]; then
echo '3.4.0beta1'
echo '3.4.0'
fi
}

Expand Down Expand Up @@ -294,6 +313,7 @@ function build_php {
make install-sapi || true
make install-binaries install-headers install-modules install-programs install-build

run_dsymutil "$prefix_dir"
rm -rf "$build_dir"
cd -
}
Expand Down Expand Up @@ -462,11 +482,12 @@ function install_xdebug {
"$php_prefix/bin/phpize"
mkdir -p "$build_dir"
cd "$build_dir"
"$xdebug_source_dir/configure" "--with-php-config=$php_prefix/bin/php-config"
CFLAGS="$CFLAGS -ggdb" "$xdebug_source_dir/configure" "--with-php-config=$php_prefix/bin/php-config"
make -j
make install
cd -

run_dsymutil "$php_prefix/lib"
rm -rf "$build_dir"
}

Expand All @@ -483,7 +504,7 @@ fi

if [[ -d /opt/homebrew/lib ]]; then
export LDFLAGS="${LDFLAGS:-} -L/opt/homebrew/lib"
export CPPFLAGS="${CPPFLAGS:-} -I/opt/homebrew/include"
export CPPFLAGS="${CPPFLAGS:-} -idirafter /opt/homebrew/include"
fi
export CXXFLAGS="${CXXFLAGS:-} -std=c++11"
export CFLAGS="${CFLAGS:-} -Wno-implicit-function-declaration"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import com.datadog.appsec.php.docker.InspectContainerHelper
import com.datadog.appsec.php.model.Span
import com.datadog.appsec.php.model.Trace
import org.junit.jupiter.api.MethodOrderer
import org.junit.jupiter.api.Order
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.TestMethodOrder
import org.junit.jupiter.api.condition.EnabledIf
Expand Down Expand Up @@ -45,12 +44,6 @@ class Symfony62Tests {
www: 'symfony62',
)

@Test
@Order(1)
void 'reported telemetry integrations are not repeated'() {

}

@Test
void 'login success automated event'() {
//The user [email protected] is already on the DB
Expand Down Expand Up @@ -113,5 +106,32 @@ class Symfony62Tests {
assert span.metrics."_dd.appsec.waf.duration" > 0.0d
assert span.meta."_dd.appsec.event_rules.version" != ''
assert span.meta."appsec.blocked" == "true"
assert span.meta."http.route" == '/dynamic-path/{param01}'
}

@Test
void 'symfony http route disabled'() {
try {
def res = CONTAINER.execInContainer(
'bash', '-c',
'''echo export DD_TRACE_SYMFONY_HTTP_ROUTE=false >> /etc/apache2/envvars;
service apache2 restart''')
assert res.exitCode == 0

HttpRequest req = container.buildReq('/dynamic-path/someValue').GET().build()
def trace = container.traceFromRequest(req, ofString()) { HttpResponse<String> re ->
assert re.statusCode() == 200
assert re.body().contains('Hi someValue!')
}

Span span = trace.first()
assert span.meta."http.route" != '/dynamic-path/{param01}'
} finally {
def res = CONTAINER.execInContainer(
'bash', '-c',
'''sed -i '/export DD_TRACE_SYMFONY_HTTP_ROUTE=/d' /etc/apache2/envvars;
service apache2 restart''')
assert res.exitCode == 0
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
framework:
test: true
session:
storage_id: session.storage.mock_file
storage_factory_id: session.storage.factory.mock_file
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ public function homeAction(Request $request)
);
}

/**
* @Route("/dynamic-path/{param01}", name="dynamic-path")
*/
public function dynamicAction(Request $request)
#[Route("/dynamic-path/{param01}", locale: "en")]
#[Route("/caminho-dinamico/{param01}", locale: "pt")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the PT route reported if locale is pt. I can't see any test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't report the routes and I don't hit this route, only then en one. The point is that there being several locales forces the route name not be a key in the route collection; instead we get route_name.locale.

public function dynamicAction(Request $request, string $param01)
{
return new Response(
'Hi!'
"Hi $param01!"
);
}
}
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ services:
test-agent:
image: ghcr.io/datadog/dd-apm-test-agent/ddapm-test-agent:latest
ports:
- "127.0.0.1:9126:8126"
- "9126:9126"
volumes:
- ./tests/snapshots:/snapshots
environment:
Expand Down
1 change: 1 addition & 0 deletions ext/configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ enum ddtrace_sampling_rules_format {
CONFIG(BOOL, DD_TRACE_LARAVEL_QUEUE_DISTRIBUTED_TRACING, "true") \
CONFIG(BOOL, DD_TRACE_SYMFONY_MESSENGER_DISTRIBUTED_TRACING, "true") \
CONFIG(BOOL, DD_TRACE_SYMFONY_MESSENGER_MIDDLEWARES, "false") \
CONFIG(BOOL, DD_TRACE_SYMFONY_HTTP_ROUTE, "true") \
CONFIG(BOOL, DD_TRACE_REMOVE_ROOT_SPAN_LARAVEL_QUEUE, "true") \
CONFIG(BOOL, DD_TRACE_REMOVE_ROOT_SPAN_SYMFONY_MESSENGER, "true") \
CONFIG(BOOL, DD_APPSEC_RASP_ENABLED , "false") \
Expand Down
133 changes: 99 additions & 34 deletions src/DDTrace/Integrations/Symfony/SymfonyIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
use DDTrace\Tag;
use DDTrace\Type;
use DDTrace\Util\Normalizer;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Contracts\Cache\ItemInterface;

class SymfonyIntegration extends Integration
{
Expand All @@ -22,6 +24,8 @@ class SymfonyIntegration extends Integration
/** @var string */
public $frameworkPrefix = SymfonyIntegration::NAME;

public $kernel;

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -355,6 +359,24 @@ function (SpanData $span) use ($class, $methodname) {
);
*/

\DDTrace\hook_method(
'Symfony\Component\HttpKernel\Kernel',
'getHttpKernel',
null,
function ($object) use ($integration) {
$integration->kernel = $object;
}
);

\DDTrace\hook_method(
'Drupal\Core\DrupalKernel',
'getHttpKernel',
null,
function ($object) use ($integration) {
$integration->kernel = $object;
}
);

\DDTrace\hook_method(
'Symfony\Component\HttpKernel\HttpKernel',
'__construct',
Expand All @@ -366,47 +388,90 @@ function () use ($integration) {
}
);

\DDTrace\trace_method(
'Symfony\Component\HttpKernel\HttpKernel',
'handle',
function (SpanData $span, $args, $response) use ($integration) {
/** @var Request $request */
list($request) = $args;

$span->name = 'symfony.kernel.handle';
$span->service = \ddtrace_config_app_name($integration->frameworkPrefix);
$span->type = Type::WEB_SERVLET;
$span->meta[Tag::COMPONENT] = SymfonyIntegration::NAME;

$rootSpan = \DDTrace\root_span();
$rootSpan->meta[Tag::HTTP_METHOD] = $request->getMethod();
$rootSpan->meta[Tag::COMPONENT] = $integration->frameworkPrefix;
$rootSpan->meta[Tag::SPAN_KIND] = 'server';
$integration->addTraceAnalyticsIfEnabled($rootSpan);

if (!array_key_exists(Tag::HTTP_URL, $rootSpan->meta)) {
$rootSpan->meta[Tag::HTTP_URL] = Normalizer::urlSanitize($request->getUri());
if (\dd_trace_env_config('DD_TRACE_SYMFONY_HTTP_ROUTE')) {
$handle_http_route = function($route_name, $request, $rootSpan) use ($integration) {
if ($integration->kernel === null) {
return;
}
if (isset($response)) {
$rootSpan->meta[Tag::HTTP_STATUS_CODE] = $response->getStatusCode();
/** @var ContainerInterface $container */
$container = $integration->kernel->getContainer();
try {
$cache = $container->get('cache.app');
} catch (\Exception $e) {
return;
}

$parameters = $request->get('_route_params');
if (!empty($parameters) &&
is_array($parameters) &&
function_exists('\datadog\appsec\push_address')) {
\datadog\appsec\push_address("server.request.path_params", $parameters);
/** @var \Symfony\Bundle\FrameworkBundle\Routing\Router $router */
$router = $container->get('router');
if (!\method_exists($cache, 'getItem')) {
return;
}
$itemName = "_datadog.route.path.$route_name";
$locale = $request->get('_locale');
if ($locale !== null) {
$itemName .= ".$locale";
}
$item = $cache->getItem($itemName);
if ($item->isHit()) {
$route = $item->get();
} else {
$routeCollection = $router->getRouteCollection();
$route = $routeCollection->get($route_name);
if ($route == null && ($locale = $request->get('_locale')) !== null) {
$route = $routeCollection->get($route_name . '.' . $locale);
}
$item->set($route);
$item->expiresAfter(3600);
$cache->save($item);
}
if (isset($route)) {
$rootSpan->meta[Tag::HTTP_ROUTE] = $route->getPath();
}
};

\DDTrace\trace_method(
'Symfony\Component\HttpKernel\HttpKernel',
'handle',
function (SpanData $span, $args, $response) use ($integration, $handle_http_route) {
/** @var Request $request */
list($request) = $args;

$span->name = 'symfony.kernel.handle';
$span->service = \ddtrace_config_app_name($integration->frameworkPrefix);
$span->type = Type::WEB_SERVLET;
$span->meta[Tag::COMPONENT] = SymfonyIntegration::NAME;

$rootSpan = \DDTrace\root_span();
$rootSpan->meta[Tag::HTTP_METHOD] = $request->getMethod();
$rootSpan->meta[Tag::COMPONENT] = $integration->frameworkPrefix;
$rootSpan->meta[Tag::SPAN_KIND] = 'server';
$integration->addTraceAnalyticsIfEnabled($rootSpan);

if (!array_key_exists(Tag::HTTP_URL, $rootSpan->meta)) {
$rootSpan->meta[Tag::HTTP_URL] = Normalizer::urlSanitize($request->getUri());
}
if (isset($response)) {
$rootSpan->meta[Tag::HTTP_STATUS_CODE] = $response->getStatusCode();
}

$route = $request->get('_route');
if (null !== $route && null !== $request) {
if (dd_trace_env_config("DD_HTTP_SERVER_ROUTE_BASED_NAMING")) {
$rootSpan->resource = $route;
$route_name = $request->get('_route');
if ($route_name !== null) {
if (dd_trace_env_config("DD_HTTP_SERVER_ROUTE_BASED_NAMING")) {
$rootSpan->resource = $route_name;
}
$rootSpan->meta['symfony.route.name'] = $route_name;
$handle_http_route($route_name, $request, $rootSpan);
}

$parameters = $request->get('_route_params');
if (!empty($parameters) &&
is_array($parameters) &&
function_exists('datadog\appsec\push_address')) {
\datadog\appsec\push_address("server.request.path_params", $parameters);
}
$rootSpan->meta['symfony.route.name'] = $route;
}
}
);
);
}

/*
* EventDispatcher v4.3 introduced an arg hack that mutates the arguments.
Expand Down
2 changes: 1 addition & 1 deletion tests/Common/TracerTestTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public function tracesFromWebRequest($fn, $tracer = null, callable $until = null
// Clearing existing dumped file
$this->resetRequestDumper();

// The we server has to be configured to send traces to the provided requests dumper.
// The web server has to be configured to send traces to the provided requests dumper.
$fn($tracer);

self::putEnv('DD_TRACE_SHUTDOWN_TIMEOUT');
Expand Down
Loading
Loading