Display discovered clients in the profiler #154

fbourigault · 2017-04-19T21:26:45Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	fixes #133
Documentation	N/A
License	MIT

Content

Auto discovered clients were not visible in the profiler. This behavior is broken since at least 1.3. I didn't test any further back to find when it was broken.

This also fixes the root cause of #133. The easier way to test it is with knplabs/github-api.

The bug root cause was caused by the stopwatch plugin which was shared between every clients. When a client was configured, the stopwatch plugin gets decorated with the ProfilePlugin and when executed, no stack plugin was executed before.

About plugins state

So we have a plugin instance per client. I don't know if it's a good or a bad thing but we probably have to be clear on what to do. I think its ok to share a plugin if it is stateless, but if the plugin is stateful it should not be shared at all (like the StackPlugin which hold the client name).

I think we should choose a way to go, shared plugins or no shared plugins. Doing the choice will probably allow us to do some refactoring in the bundle container extension, which looks like really complicated to met.

Ping @Nyholm @sagikazarmark @dbu

Nyholm · 2017-04-24T08:28:05Z

I think it is fair to share plugins. The plugins should generally be stateless.. But the StackPlugin is special. Can it be treated in a special way o should we try to make it stateless?

fbourigault · 2017-04-24T08:48:52Z

StackPlugin is already managed in a special way (i.e. not decorated with PluginClient).

I think we have to work on how plugins are decorated. If the plugin is a shared one, we have to register it once and decorate only once. If the plugin is defined in the client, from httplug catalogue (means stopwatch, redirect), we have to create an instance per client and decorate this instance.

What we can do for user created plugins? Shall we consider them like shared plugins and document that sharing a plugin not designed for can be harmful?

In the current definition, I would make all plugins from plugins.xml abstract, then iterate over the shared plugin configuration and create required plugins from abstract definition (decorated with ProfileClient if profiling is on).
Then loop over clients definition and create specific definition for httplug shipped plugins (decorated with ProfileClient).
For the user defined plugins, we have to write some logic to ensure that the service will be decorated only once with ProfileClient.

And then we add a StackPlugin as first plugin.

WDYT?

Nyholm · 2017-04-24T09:07:46Z

StackPlugin is already managed in a special way

Good

What we can do for user created plugins? Shall we consider them like shared plugins and document that sharing a plugin not designed for can be harmful

Yes.

I would make all plugins from plugins.xml abstract,

That is a BC break, right? Why don't we use the same logic for user defined plugins as we do with ours?

fbourigault · 2017-04-24T09:30:29Z

That is a BC break, right? Why don't we use the same logic for user defined plugins as we do with ours?
This is probably a BC break. I will not change that.

I will share again the stopwatch plugin and only focus this PR on fixing #133.

Nyholm · 2017-04-24T09:28:59Z

DependencyInjection/HttplugExtension.php

+        $pluginServiceId = $serviceId.'.plugin.'.$pluginName;
+
+        $definition = class_exists(ChildDefinition::class)
+            ? new ChildDefinition('httplug.plugin'.'.'.$pluginName)


There too many concats here.

ChildDefinition('httplug.plugin.'.$plug...

Same on the next line

Nyholm · 2017-04-24T09:46:24Z

Im testing this PR. The auto_discovered_client does not show up the the profiler.

fbourigault · 2017-04-24T13:40:05Z

Finally I focused this PR about displaying auto discovered clients in the profiler and fixing #133.

Nyholm

Excellent. I've tested the code and it works flawlessly.

dbu

great work, thanks!

we can merge as is, or we can do our future selves a favor and add a line to each method to explain what it does (beyond what the method name says - some of the methods do have such one-liners and i find them helpful)

dbu · 2017-04-25T06:07:22Z

DependencyInjection/HttplugExtension.php

+
+    /**
+     * @param ContainerBuilder $container
+     * @param $pluginServiceId


missing typehint

dbu · 2017-04-25T06:09:25Z

DependencyInjection/HttplugExtension.php

+     * @param string           $pluginName
+     * @param array            $pluginConfig
+     *
+     * @return string


please add something like "service id for this plugin"

fbourigault · 2017-04-25T06:15:03Z

Thanks for your review. I will add some description to extracted methods.

fbourigault · 2017-04-25T12:02:27Z

I fixed issues pointed out by @dbu. I also added one line description for new methods. Proofreading is welcome!

dbu

thanks! i went over the docs and commented where i am still confused. but we can totally merge as is, its quite some improvement in here! the comments that are left basically are things that are now more visible that i find confusing in the existing code already.

if you want to improve the phpdoc some more, please do but otherwise feel free to squash and merge.

dbu · 2017-04-26T07:17:00Z

DependencyInjection/HttplugExtension.php

-                ->getDefinition('httplug.collector.plugin_journal')
-                ->addMethodCall('setPlugins', [$name, ['httplug.plugin.stopwatch']])
-            ;
+            // Add the newstack plugin


new stack plugin? or newstack? if i guess correctly what we do here, lets do: // To profile requests, we put a stack plugin instance as first plugin in the chain, followed by the stopwatch plugin

dbu · 2017-04-26T07:19:09Z

DependencyInjection/HttplugExtension.php

+     */
+    private function configureNewStackPlugin(ContainerBuilder $container, $clientName, $serviceId)
+    {
+        $pluginServiceId = $serviceId.'.plugin.newstack';


i see that this is a refactoring and you just kept the previous name, but why do we call it "newstack"? its kind of confusing to me. its not really more new than other services, just specific to this httplug client, right?

dbu · 2017-04-26T07:19:40Z

DependencyInjection/HttplugExtension.php

+     *
+     * @param ContainerBuilder $container
+     * @param string           $clientName
+     * @param string           $serviceId


is this the service id of a httplug client? if so, lets say that.

dbu · 2017-04-26T07:20:20Z

DependencyInjection/HttplugExtension.php

+     * Configure a new StackPlugin for a client.
+     *
+     * @param ContainerBuilder $container
+     * @param string           $clientName


this is used to configure the stack plugin so that we can show the right client name in the profiler, right? maybe mention this here...

fbourigault · 2017-04-26T08:24:57Z

@dbu I improved comments. Hope this is now easier to understand.

dbu

LGTM

Nyholm requested a review from dbu April 19, 2017 21:26

sagikazarmark added the in progress label Apr 19, 2017

fbourigault added this to the Version 1.5.0 milestone Apr 19, 2017

fbourigault mentioned this pull request Apr 20, 2017

Documentation: Use for Reusable Bundles #58

Closed

fbourigault changed the title ~~[WIP] Add StackPlugin for discovered clients~~ Add StackPlugin for discovered clients Apr 24, 2017

Nyholm reviewed Apr 24, 2017

View reviewed changes

fbourigault changed the title ~~Add StackPlugin for discovered clients~~ Display discovered clients in the profiler Apr 24, 2017

Nyholm approved these changes Apr 24, 2017

View reviewed changes

dbu approved these changes Apr 25, 2017

View reviewed changes

dbu reviewed Apr 26, 2017

View reviewed changes

display auto discovered clients in the profiler

67b0210

dbu approved these changes Apr 26, 2017

View reviewed changes

fbourigault merged commit 8c98056 into php-http:master Apr 26, 2017

fbourigault deleted the fix-missing-stack branch April 26, 2017 09:11

sagikazarmark removed the in progress label Apr 26, 2017

fbourigault mentioned this pull request Jul 6, 2017

display children requests as nested in the profiler #181

Merged

Display discovered clients in the profiler #154

Display discovered clients in the profiler #154

Uh oh!

Conversation

fbourigault commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Content

About plugins state

Uh oh!

Nyholm commented Apr 24, 2017

Uh oh!

fbourigault commented Apr 24, 2017

Uh oh!

Nyholm commented Apr 24, 2017

Uh oh!

fbourigault commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Apr 24, 2017

Uh oh!

fbourigault commented Apr 24, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Apr 25, 2017

Uh oh!

fbourigault commented Apr 25, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Apr 26, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fbourigault commented Apr 19, 2017 •

edited

Loading