Extract the profiler to a new component #10374

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
9 participants
@fabpot
Owner

fabpot commented Mar 4, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR not use how to handle documentation

This pull request extracts the HTTP profiler from the HttpKernel component to a new dedicated component: HttpProfiler. This is the first step towards splitting the HttpKernel component into reusable sub-components (see #9406 for more information about this strategy).

*/
-class ConfigDataCollector extends DataCollector
+class ConfigDataCollector extends \Symfony\Component\HttpProfiler\DataCollector\ConfigDataCollector

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 4, 2014

Member

you should make these deprecated class implement the deprecated DataCollectorInterface as well to provide BC

@stof

stof Mar 4, 2014

Member

you should make these deprecated class implement the deprecated DataCollectorInterface as well to provide BC

+ "suggest": {
+ },
+ "autoload": {
+ "psr-0": { "Symfony\\Component\\HttpProfiler\\": "" }

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 4, 2014

Member

I suggest using psr-4 instead of psr-0 + target-dir, as it is cleaner (and I suggest doing the change in all components in a separate PR). target-dir was a composer hack to woraround the absence of PSR-4 previously for such case

@stof

stof Mar 4, 2014

Member

I suggest using psr-4 instead of psr-0 + target-dir, as it is cleaner (and I suggest doing the change in all components in a separate PR). target-dir was a composer hack to woraround the absence of PSR-4 previously for such case

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 4, 2014

Member

The deprecated storage classes should return an HttpKernel Profile instance to preserve BC on the return value

Member

stof commented Mar 4, 2014

The deprecated storage classes should return an HttpKernel Profile instance to preserve BC on the return value

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 4, 2014

Contributor

Maybe it was done for BC compat, but why is it HttpProfiler and not just Profiler?

Contributor

Ocramius commented Mar 4, 2014

Maybe it was done for BC compat, but why is it HttpProfiler and not just Profiler?

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 4, 2014

Owner

@Ocramius Because the profiler is tied to HttpFoundation; the main entry point is Profiler::collect($request, $response, $exception).

Owner

fabpot commented Mar 4, 2014

@Ocramius Because the profiler is tied to HttpFoundation; the main entry point is Profiler::collect($request, $response, $exception).

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 4, 2014

Contributor

I see... Guess we won't have a multi-purpose profiler in 2.x then?

Contributor

Ocramius commented Mar 4, 2014

I see... Guess we won't have a multi-purpose profiler in 2.x then?

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 4, 2014

Owner

@Ocramius Depends on the definition of "profiler". We have an HTTP profiler but also the Stopwatch component. Any references to a multi-purpose profiler in PHP or other languages?

Owner

fabpot commented Mar 4, 2014

@Ocramius Depends on the definition of "profiler". We have an HTTP profiler but also the Stopwatch component. Any references to a multi-purpose profiler in PHP or other languages?

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 4, 2014

Owner

Also, the timeline feature of the web profiler is probably also interesting.

Owner

fabpot commented Mar 4, 2014

Also, the timeline feature of the web profiler is probably also interesting.

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 4, 2014

Owner

I was also thinking about moving some classes from WebProfilerBundle to HttpProfiler. Also, now that this is a standalone component, it might be a good idea to move the Doctrine data collector here and more the Doctrine profiler templates in the web profiler. I have had a look at those yet, but if possible, that would allow out of the box support for Doctrine in the Silex WebProfiler.

Owner

fabpot commented Mar 4, 2014

I was also thinking about moving some classes from WebProfilerBundle to HttpProfiler. Also, now that this is a standalone component, it might be a good idea to move the Doctrine data collector here and more the Doctrine profiler templates in the web profiler. I have had a look at those yet, but if possible, that would allow out of the box support for Doctrine in the Silex WebProfiler.

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 4, 2014

Contributor

@fabpot I'm in the process of re-designing ZendDeveloperTools (ancient toolbar that was basically forward-ported to ZF2), and seeing this PR I had some hope for some sort of "collector" interface of some sort that wasn't tied to HTTP apps (since CLI apps also use the MVC/request/dispatch flow, in my case).

It's perfectly fine to extract the component in its own namespace, I was just hoping for something less coupled. If that's not the aim of the new component, that is perfectly fine.

Contributor

Ocramius commented Mar 4, 2014

@fabpot I'm in the process of re-designing ZendDeveloperTools (ancient toolbar that was basically forward-ported to ZF2), and seeing this PR I had some hope for some sort of "collector" interface of some sort that wasn't tied to HTTP apps (since CLI apps also use the MVC/request/dispatch flow, in my case).

It's perfectly fine to extract the component in its own namespace, I was just hoping for something less coupled. If that's not the aim of the new component, that is perfectly fine.

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 4, 2014

Member

@fabpot DoctrineBundle is already extending the DataCollector to implement more features. I agree about moving the Doctrine DataCollector to the component (though the bridge seems more appropriate), but only if we ma,nage to move the template at the same time. If we need to keep maintaining the template in DoctrineBundle, we would end up extending the class again later when we want to add a new feature because of the different release cycles

Member

stof commented Mar 4, 2014

@fabpot DoctrineBundle is already extending the DataCollector to implement more features. I agree about moving the Doctrine DataCollector to the component (though the bridge seems more appropriate), but only if we ma,nage to move the template at the same time. If we need to keep maintaining the template in DoctrineBundle, we would end up extending the class again later when we want to add a new feature because of the different release cycles

@webmozart

This comment has been minimized.

Show comment Hide comment
@webmozart

webmozart Mar 4, 2014

Contributor

Great initiative! :) Like @Ocramius, I think we should take this chance to improve the general architecture and usability of the component.

Currently, the profiler has a couple of problems:

  • A profile contains data collectors. Thus data collectors must be serializable, which is difficult if they depend on external services. In short: The data collectors have too many responsibilities (collecting data and storing data)
  • Data collectors can't inspect anything else but Request, Response and Exception objects (except with hacks, see FormDataCollector).
  • The profiler has too many responsibilities: Profiling an application and storing/loading profiles. This should be decoupled.

Let's try to redesign the component with a DDD approach. The basic idea of a profiler is to profile the run of an application. So the primary operation should be:

$profile = $profiler->profile($run);

For HTTP requests, this could be:

$run = new ApplicationRun();
$run->set('request', $request);
$run->set('response', $response);
$run->set('exception', $exception);
$run->set('forms', $forms);

$profiler->addCollector(new RequestDataCollector());
$profiler->addCollector(new ResponseTimeDataCollector());
$profiler->addCollector(new ExceptionDataCollector());
$profiler->addCollector(new FormDataCollector());

$profile = $profiler->profile($run);

The profiler then passes the run to all data collectors, which inspect the run and return their data objects (if any):

public function profile(ApplicationRun $run)
{
    $profileData = array();

    foreach ($this->collectors as $collector) {
        if (null !== ($data = $collector->collectData($run))) {
            $profileData[$data->getName()] = $data;
        }
    }

    // ApplicationProfile is more specific and easier to understand than just Profile
    // (every application has user profiles -> source of confusion)
    // This class is furthermore immutable. Profiles should not be changed after
    // profiling.
    return new ApplicationProfile($profileData);
}

The end user can then access the application profile to access the information collected by the data collectors:

$profile = $profiler->profile($run);

$data = $profile->get('request'); // returns RequestData

echo $profile->get('request')->getMethod();

// etc.

Finally, I want to store and load profiles. We can do this using the ProfileStorage. For identifying profiles, we need some sort of identifier:

$storage = new FileProfileStorage();
$storage->write($profile);
$profileId = $profile->getId();
// store $profileId somewhere

$profile = $storage->read($profileId);

So we end up with:

  • The ApplicationRun, which stores all data about the current execution of the application.
  • The DataCollectors, which extract ProfileData objects from the run.
  • The Profiler, which is composed of data collectors and converts an ApplicationRun into an ApplicationProfile.
  • The ApplicationProfile, which is composed of ProfileData objects and which may contain nested ApplicationProfiles (for sub-requests or sub-processes). Each profile is identified by an ID.
  • The ProfileStorage, which is responsible for storing and restoring ApplicationProfile instances.

I think this architecture is simpler, more extensible and easier to understand than the current architecture. The changed names (which are important for understanding the code easily) can easily be bridged in the BC classes in HttpKernel.

Contributor

webmozart commented Mar 4, 2014

Great initiative! :) Like @Ocramius, I think we should take this chance to improve the general architecture and usability of the component.

Currently, the profiler has a couple of problems:

  • A profile contains data collectors. Thus data collectors must be serializable, which is difficult if they depend on external services. In short: The data collectors have too many responsibilities (collecting data and storing data)
  • Data collectors can't inspect anything else but Request, Response and Exception objects (except with hacks, see FormDataCollector).
  • The profiler has too many responsibilities: Profiling an application and storing/loading profiles. This should be decoupled.

Let's try to redesign the component with a DDD approach. The basic idea of a profiler is to profile the run of an application. So the primary operation should be:

$profile = $profiler->profile($run);

For HTTP requests, this could be:

$run = new ApplicationRun();
$run->set('request', $request);
$run->set('response', $response);
$run->set('exception', $exception);
$run->set('forms', $forms);

$profiler->addCollector(new RequestDataCollector());
$profiler->addCollector(new ResponseTimeDataCollector());
$profiler->addCollector(new ExceptionDataCollector());
$profiler->addCollector(new FormDataCollector());

$profile = $profiler->profile($run);

The profiler then passes the run to all data collectors, which inspect the run and return their data objects (if any):

public function profile(ApplicationRun $run)
{
    $profileData = array();

    foreach ($this->collectors as $collector) {
        if (null !== ($data = $collector->collectData($run))) {
            $profileData[$data->getName()] = $data;
        }
    }

    // ApplicationProfile is more specific and easier to understand than just Profile
    // (every application has user profiles -> source of confusion)
    // This class is furthermore immutable. Profiles should not be changed after
    // profiling.
    return new ApplicationProfile($profileData);
}

The end user can then access the application profile to access the information collected by the data collectors:

$profile = $profiler->profile($run);

$data = $profile->get('request'); // returns RequestData

echo $profile->get('request')->getMethod();

// etc.

Finally, I want to store and load profiles. We can do this using the ProfileStorage. For identifying profiles, we need some sort of identifier:

$storage = new FileProfileStorage();
$storage->write($profile);
$profileId = $profile->getId();
// store $profileId somewhere

$profile = $storage->read($profileId);

So we end up with:

  • The ApplicationRun, which stores all data about the current execution of the application.
  • The DataCollectors, which extract ProfileData objects from the run.
  • The Profiler, which is composed of data collectors and converts an ApplicationRun into an ApplicationProfile.
  • The ApplicationProfile, which is composed of ProfileData objects and which may contain nested ApplicationProfiles (for sub-requests or sub-processes). Each profile is identified by an ID.
  • The ProfileStorage, which is responsible for storing and restoring ApplicationProfile instances.

I think this architecture is simpler, more extensible and easier to understand than the current architecture. The changed names (which are important for understanding the code easily) can easily be bridged in the BC classes in HttpKernel.

@sroze

This comment has been minimized.

Show comment Hide comment
@sroze

sroze Oct 17, 2014

Member

I agree with @webmozart !

Moreover, I thing that we should rewrite the Profiler::find method to allow a simple array of parameters, like the Doctrine's findBy method. For sure, with the same changes on ProfilerStorageInterface::find.
There's a simple reason: it would improve the DX to display the response code in the searchResults list because it isn't always simple to find a buggy request in the list just by its method & path...

Member

sroze commented Oct 17, 2014

I agree with @webmozart !

Moreover, I thing that we should rewrite the Profiler::find method to allow a simple array of parameters, like the Doctrine's findBy method. For sure, with the same changes on ProfilerStorageInterface::find.
There's a simple reason: it would improve the DX to display the response code in the searchResults list because it isn't always simple to find a buggy request in the list just by its method & path...

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Oct 17, 2014

Member

@sroze Allowing arbitrary criteria won't work, as you cannot ensure that all storages are supporting them. Currently, the supported criteria are well defined by the signature.

Member

stof commented Oct 17, 2014

@sroze Allowing arbitrary criteria won't work, as you cannot ensure that all storages are supporting them. Currently, the supported criteria are well defined by the signature.

@sroze

This comment has been minimized.

Show comment Hide comment
@sroze

sroze Oct 17, 2014

Member

@stof There's the same design problem with the Doctrine's findBy. That can easily be solved with ignoring or throwning an exception...
So, how would you add the response status in the profiler list results (and allowing to search on) ? Change the Profiler::find and ProfilerStorageInterface::find methods prototypes which will introduce a BC break ?

Member

sroze commented Oct 17, 2014

@stof There's the same design problem with the Doctrine's findBy. That can easily be solved with ignoring or throwning an exception...
So, how would you add the response status in the profiler list results (and allowing to search on) ? Change the Profiler::find and ProfilerStorageInterface::find methods prototypes which will introduce a BC break ?

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Oct 17, 2014

Member

@sroze Doctrine is able to find based on any field available in the entity, and it knows this list. Profiler storages are not all able to search on arbitrary fields. Database-based storages (PDO and Mongo for instance) are able to do this. However, other storages (including the default filesystem one) can only search based on fields registered in the index. and each time we want to add a field, we make the index grow larger

Member

stof commented Oct 17, 2014

@sroze Doctrine is able to find based on any field available in the entity, and it knows this list. Profiler storages are not all able to search on arbitrary fields. Database-based storages (PDO and Mongo for instance) are able to do this. However, other storages (including the default filesystem one) can only search based on fields registered in the index. and each time we want to add a field, we make the index grow larger

@sroze

This comment has been minimized.

Show comment Hide comment
@sroze

sroze Oct 17, 2014

Member

@stof I can't go against that as I totally agree. By the way, in that case, what would you do to add the response status code in stored tokens to be able to see it in the token list ? That's the real point I think we should solve to improve DX, as it'll make it easier to identify the searched request (and filtering by status code moreover).

Member

sroze commented Oct 17, 2014

@stof I can't go against that as I totally agree. By the way, in that case, what would you do to add the response status code in stored tokens to be able to see it in the token list ? That's the real point I think we should solve to improve DX, as it'll make it easier to identify the searched request (and filtering by status code moreover).

@barryvdh

This comment has been minimized.

Show comment Hide comment
@barryvdh

barryvdh Jan 8, 2015

Contributor

Is the point of this PR also to make the WebProfiler more of a stand-alone tool, usable by other frameworks that use the HttpFoundation (or even other request objects)? Cause that would be nice :)

Contributor

barryvdh commented Jan 8, 2015

Is the point of this PR also to make the WebProfiler more of a stand-alone tool, usable by other frameworks that use the HttpFoundation (or even other request objects)? Cause that would be nice :)

@barryvdh

This comment has been minimized.

Show comment Hide comment
@barryvdh

barryvdh Jan 8, 2015

Contributor

@fabpot Any references to a multi-purpose profiler in PHP or other languages?

These 2 are both multi-purpose that do pretty much the same, run some collectors on the request, store the result and output it somewhere:
https://github.com/itsgoingd/clockwork
https://github.com/maximebf/php-debugbar

(And Z-ray, but that's not really PHP but in the Zend server)

Contributor

barryvdh commented Jan 8, 2015

@fabpot Any references to a multi-purpose profiler in PHP or other languages?

These 2 are both multi-purpose that do pretty much the same, run some collectors on the request, store the result and output it somewhere:
https://github.com/itsgoingd/clockwork
https://github.com/maximebf/php-debugbar

(And Z-ray, but that's not really PHP but in the Zend server)

@mickaelandrieu

This comment has been minimized.

Show comment Hide comment
@mickaelandrieu

mickaelandrieu May 4, 2015

Contributor

@fabpot do you have a "functional" roadmap on what do we need to do to finish this pull request ?
I want to help :)

Contributor

mickaelandrieu commented May 4, 2015

@fabpot do you have a "functional" roadmap on what do we need to do to finish this pull request ?
I want to help :)

@jelte

This comment has been minimized.

Show comment Hide comment
@jelte

jelte May 21, 2015

Contributor

I mostly agree with @webmozart , but I think the "ApplicationRun" is a bit over engineering.
as I see DataCollectors more as Observers of the application which subscribe to specific events and Collect the data based on those events being triggered. This method is already being used by current DataCollectors such as RequestDataCollector ( to collect the controllers ).
The issue I see with the ApplicationRun is that again you create something that is passed along to the DataCollectors which some or even most won't use.

I've been going through the DataCollectors and a lot of them never use the Request, Response or even Exception. ( you can check my experimentation at https://github.com/jelte/symfony/pull/1/files )
Having this dependency makes it impossible to reuse them for other data collection.

Making DataCollectors not strictly dependant on them would allows for some reuse when creating other Profilers, such as a ConsoleProfile.

The DataCollector could then also be reused in other places and make the profiler and debug bar reusable in other projects.

What I basically did is make DataCollectors that require the Request dependant on the RequestStack and those that require the Response listen for the KernelEvent::RESPONSE.

I think even the Profile::collect method can lose its arguments. By injecting the RequestStack the profile could get the required info from the RequestStack->getMasterRequest(). But I haven't played around with this yet.

Contributor

jelte commented May 21, 2015

I mostly agree with @webmozart , but I think the "ApplicationRun" is a bit over engineering.
as I see DataCollectors more as Observers of the application which subscribe to specific events and Collect the data based on those events being triggered. This method is already being used by current DataCollectors such as RequestDataCollector ( to collect the controllers ).
The issue I see with the ApplicationRun is that again you create something that is passed along to the DataCollectors which some or even most won't use.

I've been going through the DataCollectors and a lot of them never use the Request, Response or even Exception. ( you can check my experimentation at https://github.com/jelte/symfony/pull/1/files )
Having this dependency makes it impossible to reuse them for other data collection.

Making DataCollectors not strictly dependant on them would allows for some reuse when creating other Profilers, such as a ConsoleProfile.

The DataCollector could then also be reused in other places and make the profiler and debug bar reusable in other projects.

What I basically did is make DataCollectors that require the Request dependant on the RequestStack and those that require the Response listen for the KernelEvent::RESPONSE.

I think even the Profile::collect method can lose its arguments. By injecting the RequestStack the profile could get the required info from the RequestStack->getMasterRequest(). But I haven't played around with this yet.

@DavidBadura

This comment has been minimized.

Show comment Hide comment
@DavidBadura

DavidBadura May 21, 2015

Contributor

It would be awesome if the Web Profiler would be a stand-alone tool :-)

Contributor

DavidBadura commented May 21, 2015

It would be awesome if the Web Profiler would be a stand-alone tool :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment