Extract the profiler to a new component

[jelte]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#11263
License	MIT
Doc PR	symfony/symfony-docs#5422

This pull request is a replacement for #10374

This pull request extracts the HTTP profiler from the HttpKernel component to a new dedicated component: Profiler.

The goals are to improve the general architecture and usability of the component as stated by @webmozart ( #10374 (comment) )
as it stands now no BC is broken as the original DataCollectors are still supported by the new Profiler.

New DataCollectors and Profiler no longer require a Request.
DataCollectors are should be able to collect data from other sources, which will prevent some ugly hacks that were previously required, they will still require to listen to events to collect the data, I don't see a way around this, but this doesn't seem a big issue.

If a DataCollector requires the Request, it should have the RequestStack and get it from there, the Responses can be added just like Controllers are added, by subscribing to the Kernel Events.

Changes

• Profiler is no longer capable of searching or retrieving profiles, developers should use the ProfilerStorage directly for these functionalities.
• DataCollectors are no longer added to the Profile. A ProfileDataInterface object is added instead.
• DataCollectors no longer have a name.
• Saving the profile is independent if its indexes, this allows for storing other types of Profiles while using the same Profiler and ProfilerStorage. ( for example Console Commands )
• Many of the DataCollectors have been moved to their respective Component under their Profiler namespace.

Profiling

$profiler = new Profiler($storageInterface);
$profiler->addCollector(new RequestDataCollector($requestStack));
$profiler->addCollector(new ExceptionDataCollector());
$profiler->addCollector(new FormDataCollector());

// Runtime data is collected
$profile = $profiler->profile();

$requestData = $profile->get('request');

// Late data is collected
$profiler->save($profile, array('url' => 'http://localhost/', 'ip' => '127.0.0.1', 'method' => 'GET', 'status_code' => 200, 'profile_type' => 'http');

BC Breaks

DataCollectors

DataCollectors require getCollectedData.

solution:
commented the method in the DataCollectorInterface and added a fallback when the method does not exists.
This will the use the GenericProfileData to encapsulate the DataCollector, which will trigger a deprecated notice.

ProfilerStorages

ProfilerStorages require findBy which replaces the find method.

solution:
commented the method in the ProfilerStorageInterface.

Todo's

• add additional tests to increase coverage.
• add README.md
• Test with Symfony demo
• Convert Bundle & Bridge DataCollectors (if that does not break BC)
• ConsoleProfiler
• Move DataCollector to their related Component or Bridge
• DoctrineDataCollector: Move collected data from DoctrineBundle to DoctrineBridge
• Refactor FormDataCollector
• Refactor DumpDataCollector as it has to many responsibilities
• add tests for MysqlProfileStorage
• Make ProfileData Serializable
• Rename DataCollectorInterface::collect to getCollectedData
• Remove getName from DataCollectorInterface
• Remove storage methods (load, find, purge) from Profiler
• Remove Collectors from Profile
• Remove export & import from Profiler
• move all deprecated code to the (deprecated) subclass.
• Check for BC breaks ( and fix where possible )
• add phpdocs where missing
• squash pull request
• submit changes to the documentation

Special thanks to @stof & @dosten for helping me out on the initial pull request ( #14763 )

[dosten]

Why you do not move this method definition to the new DataCollectorInterface and only keep this interface empty and extending the new one?

[jelte]

There are 2 reason for this, for one the new DataCollectorInterface doesn't have a collect function any more.
RuntimeDataCollectorInterface replace the old DataCollectorInterface, as I noticed some if not most use the LateDataCollectorInterface and have no use for a collect method.
For that reason I have removed the collect method from DataCollectorInterface.

The other reason is that the new profiler does not require a Request and Response, but to maintain BC I had to leave this in.

[mickaelandrieu]

this is probably
"symfony/profiler": "~2.8|~3.0.0"

[mickaelandrieu]

@jelte don't give up please, I need this component too and I will be happy to help you 👍

[jelte]

@mickaelandrieu I won't, a lot of the comments are issues coming from the original files, any others are good learning for me.

[phansys]

I think you should use PHP-CS-FIXER to solve majority of your CS issues. It should leave a clear code to make the review easy.

[jelte]

For some of the DataCollectors I have the feeling they do not belong in the Profiler:

• DumpDataCollector => VarDumper or DebugBundle, the DumpListener from HttpKernel should probably be moved as well.
• EventDataCollector => EventDispatcher
• LoggerDataCollector => MonologBridge, the DebugLoggerInterface should also be moved ( should be done carefully as this is used in the ExceptionListener )
• RequestDataCollector => HttpKernel
• RouteDataCollector => Routing

This is already done for the FormDataCollector and SecurityDataCollector.

Collectors that would remain in the Profiler are:

• ConfigDataCollector. I am considering splitting this one to ProjectDataCollector, SymfonyDataCollector and ConfigDataCollector, but this will have some impact on the WebProfilerBundle.
• MemoryDataCollector
• TimeDataCollector

[mickaelandrieu]

@jelte totaly agree but I disagree to make new strong dependencies between the components.

Maybe FrameworkBundle is the better place for theses collectors ?

[jelte]

@mickaelandrieu I would keep them in their respective Bundle or Bridge if one exists, rather than dumping everything in the FrameworkBundle.

• DumpDataCollector => DebugBundle
• EventDataCollector => FrameworkBundle
• LoggerDataCollector => MonologBridge
• RequestDataCollector => FrameworkBundle
• RouteDataCollector => FrameworkBundle

[mickaelandrieu]

@jelte agree :) /c @fabpot what do you think ?

[stof]

Please don't move them into bundles. It would make the profiler unusable for people using the components (Silex for instance).

[mickaelandrieu]

@stof humm, how about a symfony-collectors package then ?

[jelte]

@stof I understand, that's why I suggested to keep them in the component they belong to (like what is done with the FormProfiler at the moment).
@mickaelandrieu this will just move the issue from profiler to symfony-collectors (creating a bundle that has dependency to everything).

[aitboudad]

we could remove managers and connections properties, we don't need them here.

[stof]

For the Doctrine bridge, the new collector might be a good time to move the extra data collected in DoctrineBundle to the bridge itself btw.

[stof]

Well, once we have a dedicated component for the profiler, component profilers could indeed go in each component. this would be consistent with the fact that ContainerAwareDispatcher is in the EventDispatcher rather than the DI component for instance.
Each component with a profiling collector would then have an optional dependency on the Profiler component (optional because using the collector would be optional of course, and useful only for people using the profiler component)

[jelte]

At the moment, I have resolved all deprecation notices.
(Build still fails, but they are the same failures as the 2.8 branch)

[jelte]

@stof @fabpot

ping @symfony/deciders can someone take a look at this contribution? At least to say if this may have a chance to be merged or not?

[Tobion]

@jelte it most likely will not be part of 2.8/3.0 as we are trying to stabilize now and not make big changes. But considering the amount of work done here, I assume it's worth to merge this in 3.1 later.

[ghost]

As I currently understand I need to enable debug on the AppKernel to use the time measure in the Profiler, right?

My idea was to enable the profiler in our "testing" environment in combination with enabled caching to check the performance of our application. The issue is that if I enable 'debug' in the AppKernel I will trigger a lot of other things which will result in non-caching (for example the CSS files are generated and not cached).

Will this new Profiler component fix this issue and will it provide a good solution for this?

[jelte]

@Tobion Figured as much, just wanted confirmation, will keep this up-to-date as frequently as possible.

[jelte]

@JHGitty You can already do this now by defining a second Profiler and ProfilerListener and only adding the TimeCollector to it.
Just put it in a separate xml which is only loaded on your "testing" environment.

     <service id="test.profiler_listener" class="Symfony\Component\HttpKernel\EventListener\ProfilerListener">
            <tag name="kernel.event_subscriber" />
            <argument type="service" id="test.profiler" />
            <argument type="service" id="test.request_matcher"/>
            <argument>false</argument>
            <argument>false</argument>
            <argument type="service" id="request_stack" />
        </service>

        <service id="test.profiler" class="Symfony\Component\HttpKernel\Profiler\Profiler">
            <argument type="service" id="test.profiler.storage" />
            <call method="add">
                <argument type="service" id="data_collector.time"/>
            </call>
        </service>

Idea would be just to config what you'd need and nothing more. This would only get you the data being generated.

[ghost]

@jelte Is there any documentation about this? I think somebody should add a documentation page about this. I don't think that I am the only person who like to check performance with enabled cache.

[stof]

@jelte you should probably use a separate time collector to avoid issues (collectors are stateful)

@JHGitty note that registering the time panel alone will not help much: most places registering data into the stopwatch component are controlled by the debug mode in FrameworkBundle: wrapping the event dispatcher into a traceable one to log timing info comes at a cost, which is why it is only done in debug mode (where we add more debugging info at the expense of performance).
The symfony profiler is a debugging profiler (to know what happens in your request), but not a timing performance profiler (for which the profiling overhead must be as small as possible). You should use a real performance profiler for that (for instance, https://blackfire.io which is free for the basic features).

Anyway, this should be discussed elsewhere than in this PR as it is unrelated (and the PR discussion should stay focused on reviewing the PR)

[pyrech]

Now that 3.0 was released and 3.1 development is in progress, is this PR still considered by the core team? @jelte already did an awesome job but the PR now obviously needs to be rebased.

[mickaelandrieu]

@pyrech we can't ask to @jelte to maintain a fork for years too.

Before ask for rebase, maybe someone from the core team could say if this contribution could be accepted or not?

Other way to do: fork once and for all, and make a bundle.

[Taluu]

A bundle is not a solution... but making a real lib is (as all components can be used independently)

[mickaelandrieu]

@Taluu sure I saw this bundle as an implementation of this fork (which is already a library/component)

[jelte]

Just to give everyone a little update,
I just started to rebase this on the current 3.0, I will update as soon as possible.

[Shine-neko]

@jelte ?

[Koc]

Fixes #21141

[Nek-]

in 4.0 ?

[Nek-]

4.0 ?

[Shine-neko]

@fabpot planned for 4.0 ?

[fabpot]

I'm closing this one. I'm not convinced that we should have a decoupled profiler. The profiler does the job as is and the fact it's tied to Request/Response is part of the design that I don't want to change. I know that it took time for me to make that decision, but after all this time, I still think that's not something that needs to be done. Thanks for working on this.

[Wirone]

This would be soooooooo helpful and I am so disappointed by @fabpot's decision :( Some applications doesn't even have HTTP context, they're 100% CLI and profiling them is a must have. I don't understand why it was rejected, at this point it would be probably crucial..

Feel bad for you, @jelte :(