[HttpClient] Added TraceableHttpClient and WebProfiler panel #30494

jeremyFreeAgent · Mar 8, 2019

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

That PR adds TraceableHttpClient and the DataCollector.

Needs #30419 for the integration of HttpClient in FrameworkBundle.

Preview:

src/Symfony/Bundle/FrameworkBundle/DataCollector/HttpClientDataCollector.php

src/Symfony/Bundle/FrameworkBundle/HttpClient/TraceableHttpClient.php

src/Symfony/Bundle/FrameworkBundle/Resources/views/Collector/http_client.html.twig

src/Symfony/Bundle/FrameworkBundle/HttpClient/TraceableHttpClient.php

HeahDude

Thank you for that PR!

src/Symfony/Bundle/FrameworkBundle/DataCollector/HttpClientDataCollector.php

src/Symfony/Bundle/FrameworkBundle/HttpClient/TraceableHttpClient.php

src/Symfony/Bundle/FrameworkBundle/Resources/views/Collector/http_client.html.twig

nicolas-grekas · Mar 17, 2019

Unlocked: FrameworkBundle integration PR is now merged, see #30419

jeremyFreeAgent · Mar 17, 2019

@nicolas-grekas great news thanks!

jeremyFreeAgent · Apr 5, 2019

Thank you @nicolas-grekas for the OnProgress trick.

src/Symfony/Bundle/FrameworkBundle/Resources/views/Collector/http_client.html.twig

src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.xml

IonBazan · Jun 7, 2019

@jeremyFreeAgent Do you need any help on this?

src/Symfony/Bundle/FrameworkBundle/HttpClient/TraceableHttpClient.php

src/Symfony/Bundle/FrameworkBundle/DataCollector/HttpClientDataCollector.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/collectors.xml

jeremyFreeAgent · Jun 11, 2019

Sorry guys for the delay. I've to focus on work these past weeks.

I rework missing things that week.

Thanks for the up @IonBazan

IonBazan · Jun 18, 2019

Thing to consider: using Stopwatch to track the timing.

nicolas-grekas · Jun 19, 2019

I squashed all commits and made some changes to show what we can collect.
One missing step for this PR: add tests please.
Note that the displayed options are the ones passed to the "request" method. Any others set by scoping clients are not shown. Of course, the response info will still contain their side-effects when there is one visible (eg added headers, absolute URL, etc)

For future PRs (let's make this one mergeable asap):

the panel UI could be improved I suppose
putting requests in the "Performance" tab would be great (all timings are already in the collected "info")
response bodies are not collected yet
when a transport error occurs, (ie the "error" info is populated) we should maybe have a better display too?
add "copy as curl" button

Note that it should be possible to collect a response that didn't complete yet (eg http_code is 0), since everything is lazy. The twig file shouldn't break when this happens.

Here is an example output:

IonBazan · Jun 20, 2019

@nicolas-grekas Thank you for your changes. To be honest, I am not a fan of simply dumping all the collected data in profiler UI. For example, headers and performance information should be presented in more "structured" way. What do you think?

I will add tests by the end of this week to make it mergeable 😄

nicolas-grekas · Jun 20, 2019

@IonBazan sure, that's why it's the 1st item in my list for a future PRs. For headers, don't forget about redirections (try one you'll see what I mean when working on this). Waiting for the tests, thanks!

tsantos84 · Jun 24, 2019

Sometimes we need to make lot of external requests and the profiler can have a big scroll to show all the information of all requests. A simple suggestion is to add a collapse button (like in messager panel) in each collected request and start all of them collapsed excepted the first one of each configured http client. To help quickly see the status of each request (considering it is hide) you can add the status code before the method on request's title.

tsantos84 · Jun 24, 2019

Collapse in/out of messenger panel.

nicolas-grekas · Jun 26, 2019

One more idea added to my list above: add a copy-as-curl button next to each request in the profiler panel.

dunglas · Jun 28, 2019

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -1898,6 +1908,19 @@ private function registerHttpClientConfiguration(array $config, ContainerBuilder
                    ->setArguments([new Reference('http_client'), [$scope => $scopeConfig], $scope]);
            }

+            if ($debug) {


It could be interesting to add a tag to the service a this point, and to do the wiring logic in a compiler pass for all services having this tag. It will allow bundles to easily plug their own clients in the profiler.

For instance, we have this use case in API Platform.

I'm almost done except your comment.

To be sure to get your point I looked the Messenger stuff. So we are ok to add a tag to all http client here https://github.com/symfony/symfony/blob/4.4/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1946-L1953
Like http_client.client ?

Then adding a HttpClientPass to make the call to the dataCollector in all service tagged with http_client.client. (if data_collector is registered of course).

Right ?

fabpot · Jul 8, 2019

@jeremyFreeAgent What's the next step? Can we merge something fast and iterate from there?

fabpot · Aug 3, 2019

src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.xml

+
+        <service id="debug.http_client" class="Symfony\Component\HttpClient\TraceableHttpClient" decorates="http_client">
+            <argument type="service" id="debug.http_client.inner" />
+        </service>


I would move these definitions to a http_client_debug.xml file like done for other components.

tyx · Aug 6, 2019

Hi guys, I had a talk with @jeremyFreeAgent and I will help on this PR.

Apparently, we only miss test to merge ? And then continue to work with small PR about refinements ?

On my side what is the best way to work with this ongoing PR ? Start another PR by adding the jeremy's commit or we can do better ?

nicolas-grekas · Aug 6, 2019

you could create a new PR that would start with the commit in this PR, and add yours on top

fabpot · Aug 6, 2019

And please, can you take the remaining comments into accounts?

chalasr · Aug 19, 2019

Closing in favor of #33015. Thanks for your work @jeremyFreeAgent

nicolas-grekas · Aug 23, 2019

See also #33311

… panel (jeremyFreeAgent) This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] Added TraceableHttpClient and WebProfiler panel | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Replace #30494 I added : - tests - move debug services declaration in dedicated `http_client_debug.xml` file - rename stuff to follow messenger data collector stuff - add CompilerPass to allow bundle to trace their own http client I didn't add all @nicolas-grekas requests on UI profiler. I will continue to make more PR after this one. IMO everything looks fine to make a first merge except one strange behavior that I am not sure to get : When making a sub request : - we still have the http_client parent data. (like messenger, but currently I did not see anything in code that could avoid that, so different topic I guess). - The data collected are already "converted" to VarDumper data, so I have errors when trying to do all the unset stuff in the TraceableHttpClient. Is it for this reason, some collector use `lateCollect` ? Should we also move to lateCollect in the `HttpClientDataCollector` ? But I'm still new on this subject but glad to help, so feel free to request more changes ! Commits ------- 5164001 [HttpClient] Added TraceableHttpClient and WebProfiler panel

carsonbot added Status: Needs Review Feature labels Mar 8, 2019

nicolas-grekas added this to the next milestone Mar 8, 2019

jeremyFreeAgent changed the title ~~❤️ Added TraceableHttpClient and DataCollector~~ Added TraceableHttpClient and DataCollector Mar 8, 2019

nicolas-grekas changed the title ~~Added TraceableHttpClient and DataCollector~~ [HttpClient] Added TraceableHttpClient and DataCollector Mar 8, 2019

chalasr added the HttpClient label Mar 8, 2019

OskarStark requested changes Mar 9, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 9, 2019

ro0NL reviewed Mar 9, 2019

View reviewed changes