Add http.route tag to SymfonyIntegration.php #2992

cataphract · 2024-12-12T02:26:55Z

Description

Adds http.route to symfony. Supersedes #2710 and #2477

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-12-12T02:58:57Z

Codecov Report

Attention: Patch coverage is 0% with 74 lines in your changes missing coverage. Please review.

Project coverage is 56.18%. Comparing base (52336c9) to head (04884ce).

Files with missing lines	Patch %	Lines
...DTrace/Integrations/Symfony/SymfonyIntegration.php	0.00%	74 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (52336c9) and HEAD (04884ce). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (52336c9) HEAD (04884ce)

tracer-php 11 10

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #2992       +/-   ##
=============================================
- Coverage     74.80%   56.18%   -18.62%     
- Complexity     2781     2789        +8     
=============================================
  Files           112      139       +27     
  Lines         11017    15211     +4194     
  Branches          0     1022     +1022     
=============================================
+ Hits           8241     8547      +306     
- Misses         2776     6112     +3336     
- Partials          0      552      +552

Flag	Coverage Δ
appsec-extension	`67.99% <ø> (?)`
tracer-php	`51.76% <0.00%> (-23.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...DTrace/Integrations/Symfony/SymfonyIntegration.php	`0.00% <0.00%> (-82.86%)`	⬇️

... and 46 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52336c9...04884ce. Read the comment docs.

pr-commenter · 2024-12-12T03:00:01Z

Benchmarks [ tracer ]

Benchmark execution time: 2024-12-20 16:32:12

Comparing candidate commit 7effdac in PR branch glopes/symfony-route with baseline commit 620af85 in branch master.

Found 1 performance improvements and 2 performance regressions! Performance is the same for 175 metrics, 0 unstable metrics.

scenario:PDOBench/benchPDOBaseline

🟥 execution_time [+4.016µs; +12.379µs] or [+2.172%; +6.695%]

scenario:PDOBench/benchPDOBaseline-opcache

🟥 execution_time [+9.749µs; +17.454µs] or [+5.480%; +9.811%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache

🟩 execution_time [-510.670ns; -238.930ns] or [-6.896%; -3.226%]

github-actions · 2024-12-12T14:42:11Z

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

If you need to update snapshots, please refer to CONTRIBUTING.md

PROFeNoM · 2024-12-17T13:08:40Z

Makefile

@@ -913,7 +913,6 @@ TEST_WEB_81 := \
 	test_web_nette_30 \
 	test_web_slim_312 \
 	test_web_slim_4 \
-	test_web_symfony_52 \


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

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

PROFeNoM

FYI, I can see the following warning/error from our CI: failure.

On a similar note, the Symfony 3.0 (:skull:) tests seems not to have the http.route tag: failure

estringana · 2024-12-17T14:20:34Z

I am missing tests with optional parameters https://symfony.com/doc/current/routing.html#optional-parameters, more than one locale https://symfony.com/doc/current/routing.html#localized-routes-i18n

estringana · 2024-12-20T14:39:07Z

src/DDTrace/Integrations/Symfony/SymfonyIntegration.php

+            }
+            $rootSpan->meta['symfony.route.name'] = $route_name;
+
+            // the rest is for determining http.route


I'd rather extract then all the code below this comment to its own function. Comments sooner or later lie. Just my opinion, not blocking the pr or anything

I've refactored it

estringana · 2024-12-20T14:40:42Z

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

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


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

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.

cataphract requested review from a team as code owners December 12, 2024 02:26

cataphract mentioned this pull request Dec 12, 2024

Add http.route tag to SymfonyIntegration.php #2710

Closed

2 tasks

cataphract force-pushed the glopes/symfony-route branch 4 times, most recently from 6a1adeb to b32dca7 Compare December 12, 2024 02:52

cataphract force-pushed the glopes/symfony-route branch 2 times, most recently from 1c181cf to 387a651 Compare December 12, 2024 14:41

cataphract force-pushed the glopes/symfony-route branch 6 times, most recently from 644c737 to ddbe994 Compare December 16, 2024 21:27

PROFeNoM reviewed Dec 17, 2024

View reviewed changes

cataphract force-pushed the glopes/symfony-route branch from ddbe994 to 7233cec Compare December 17, 2024 13:52

cataphract force-pushed the glopes/symfony-route branch from 7233cec to 0ee8ad9 Compare December 18, 2024 15:14

estringana reviewed Dec 20, 2024

View reviewed changes

Julian-Louis and others added 2 commits January 15, 2025 17:19

Add http.route tag to SymfonyIntegration.php

9dde3bf

symfony http.route: use existing hook instead

a40badc

cataphract added 5 commits January 15, 2025 17:19

Cache routes

ca38a54

Support localized routes

9fbe7c6

Fix symfony 3.0 test

537f1f1

Small refactoring

7010b37

Add option to disable http.route in symfony

04884ce

cataphract force-pushed the glopes/symfony-route branch from ee8946d to 04884ce Compare January 15, 2025 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add http.route tag to SymfonyIntegration.php #2992

Add http.route tag to SymfonyIntegration.php #2992

cataphract commented Dec 12, 2024

codecov-commenter commented Dec 12, 2024 •

edited

Loading

pr-commenter bot commented Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024

PROFeNoM Dec 17, 2024

cataphract Dec 17, 2024

PROFeNoM left a comment

estringana commented Dec 17, 2024

estringana Dec 20, 2024

cataphract Dec 20, 2024

estringana Dec 20, 2024

cataphract Dec 20, 2024

Add http.route tag to SymfonyIntegration.php #2992

Are you sure you want to change the base?

Add http.route tag to SymfonyIntegration.php #2992

Conversation

cataphract commented Dec 12, 2024

Description

Reviewer checklist

codecov-commenter commented Dec 12, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Dec 12, 2024 • edited Loading

Benchmarks [ tracer ]

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOBaseline-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache

github-actions bot commented Dec 12, 2024

Snapshots difference summary

PROFeNoM Dec 17, 2024

Choose a reason for hiding this comment

cataphract Dec 17, 2024

Choose a reason for hiding this comment

PROFeNoM left a comment

Choose a reason for hiding this comment

estringana commented Dec 17, 2024

estringana Dec 20, 2024

Choose a reason for hiding this comment

cataphract Dec 20, 2024

Choose a reason for hiding this comment

estringana Dec 20, 2024

Choose a reason for hiding this comment

cataphract Dec 20, 2024

Choose a reason for hiding this comment

codecov-commenter commented Dec 12, 2024 •

edited

Loading

pr-commenter bot commented Dec 12, 2024 •

edited

Loading