[FrameworkBundle][HttpKernel] Reset profiler #24289

derrabus · 2017-09-22T17:56:08Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#18244
License	MIT
Doc PR	N/A

This PR adds the ability to reset the profiler between requests. Furthermore, the profiler service has been tagged with the new kernel.reset tag from #24155. For this, I had to readd the ability to define multiple reset methods for a service.

Note: This PR requires twigphp/Twig#2560.

nicolas-grekas

Thanks for working on this.

nicolas-grekas · 2017-09-23T15:45:22Z

UPGRADE-3.4.md

+---------------
+
+ * Implementing `TraceableEventDispatcherInterface` without the method
+   `clearCalledListeners()` is deprecated and will be unsupported in 4.0.


If we can remove this deprecation, that'd be better IMHO, we don't this method in the interface to method. The tag doesn't need it, and when we wire the service, we know their exact type, so their set of methods also, isn't it?
(same for the other interfaces).
If we need a contract for a "reset" method, I'd suggest adding a dedicate interface for it instead (there could be one per component when needed, didn't look precisely)

The intention behind adding those methods to the interfaces was that each of them is designed to collect request-related data. There should not be a use-case of implementing them without the possiblity of resetting that data.

As it is implemented right now, the data collectors reset any helper classes they use for collecting their data. This way, you can easily reset the profiler, even if you're not using the kernel.reset feature.

nicolas-grekas · 2017-09-23T15:50:58Z

src/Symfony/Bridge/Monolog/Logger.php

@@ -46,6 +46,16 @@ public function countErrors()
    }

    /**
+     * {@inheritdoc}
+     */
+    public function clear()


should be called "reset" for consistency

Actually, I chose clear because that's how monolog calls similar functionality.

https://github.com/Seldaek/monolog/blob/fd8c787753b3a2ad11bc60c063cff1358a32a3b4/src/Monolog/Handler/TestHandler.php#L76

nicolas-grekas · 2017-09-23T15:52:49Z

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

+        $profilerDefinition = $container->getDefinition('profiler');
+        $profilerDefinition->addTag('kernel.reset', array('method' => 'reset'));
+        if ($config['collect']) {
+            $profilerDefinition->addTag('kernel.reset', array('method' => 'enable'));


Can't this be handled in the reset method itself instead? We should try harder to not have to call several methods to reset (and thus revert the multi-tags possibility).

If I wanted to do this, the profiler would need to know its initial state (enabled or disabled) so it can reset itself to that state. I don't feel comfortable with saving that information inside the profiler.

ok, still look suspicious to me:
in a kernel loop, re-enabling at reset time means collecting timings from end of previous request to end of next request, isn't it? But next request might come very long after, so timings won't mean anything.
Btw, the Twig profiler has the same issue, don't you think? I mean, calling "enter()" at reset time also?
Shouldn't this re-enabling happen at kernel.request time instead?

nicolas-grekas · 2017-09-23T15:53:22Z

src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php

@@ -214,6 +214,11 @@ public function getNotCalledListeners()
        return $notCalled;
    }

+    public function clearCalledListeners()


should be called "reset"

Okay, I'll change it.

nicolas-grekas · 2017-09-23T16:02:03Z

src/Symfony/Bridge/Twig/DataCollector/TwigDataCollector.php

@@ -44,6 +44,17 @@ public function collect(Request $request, Response $response, \Exception $except
    /**
     * {@inheritdoc}
     */
+    public function reset()
+    {
+        // TODO: Twig PR pending. Update dependencies accordingly.


can be done now to me

PR on Twig has been merged now and new releases are out for both 1.x and 2.x versions.

Dependencies updated, comment removed.

derrabus · 2017-09-30T22:46:17Z

I've rebased the branch in order to resolve merge conflicts.

derrabus · 2017-09-30T22:46:40Z

@nicolas-grekas Lets continue the discussion from #24289 (comment) here.

in a kernel loop, re-enabling at reset time means collecting timings from end of previous request to end of next request, isn't it?

Since the event handler runs very late on the kernel.terminate event, there should not be much that runs after it. In fact, there must not be any code run after the reset listener that feeds a data collector. The profiler for the next request would be tainted otherwise. That's why in #23984 (comment) I noted that kernel.terminate might not be the best place for the whole reset feature.

But next request might come very long after, so timings won't mean anything.

Timings are done by the stopwatch. As of #24193, we're resetting the stopwatch, but that only clears all collected timings from it. Resetting does not start a new timing.

The only remaining issue would be the $startTime propery of Symfony\Component\HttpKernel\Kernel. This propery is currently set in the constructor and I haven't found a good way to reset it. PHP-PM has a workaround for this in place:

https://github.com/php-pm/php-pm-httpkernel/blob/b373b0384f74e72d5cd8e8b3bef3f3ae08fd0dfa/Bootstraps/Symfony.php#L99-L100

My best guess would be to move setting $startTime to the very top of the handle() method, but I don't know if that might have some impact elsewhere.

Btw, the Twig profiler has the same issue, don't you think? I mean, calling "enter()" at reset time also?

That's what happens in the Twig Profile's constructor and it would be pretty strange if we leave the class after reset in a different state than after construction. Also, the Twig profile that we have in the service container is (as far as I can tell) only used for appending child profiles to it, and not for timing purposes.

derrabus · 2017-09-30T22:48:30Z

I've tested this branch with the Symfony demo application and PHP-PM. I've configured PHP-PM to use a single worker to make sure I'm really testing my reset feature. I've also removed the workaround code from PHP-PM's Symfony adapter locally:

https://github.com/php-pm/php-pm-httpkernel/blob/b373b0384f74e72d5cd8e8b3bef3f3ae08fd0dfa/Bootstraps/Symfony.php#L128-L198

So far, the profiler looked okay to me.

nicolas-grekas · 2017-10-04T18:18:36Z

Just pushed https://github.com/nicolas-grekas/symfony/tree/3.4-reset-profiler
see last commit

nicolas-grekas · 2017-10-05T04:27:30Z

Some comments posted in this last commit:
nicolas-grekas@10c1691

@derrabus If OK, I invite you to cherry-pick it and squash it with yours. Then it'll LGTM.

derrabus · 2017-10-05T08:15:18Z

@nicolas-grekas About your comment there:

why should we care at all about resetting the enabled/disabled state?

Because the profiler is pretty much unusable, if we don't re-enable it.

That's not a leak, so doesn't fit "reset" use case.

I wouldn't see this feature for leaks only. The main issue with the profiler is that if the kernel handles multiple requests, the profiler partly still contains data from previous requests. Because of this, it displays inaccurate data. Slowly leaking memory is more or less a side effect of this and not such a big deal since the profiler is supposed to be disabled on prod anyway.

If this is for reverting to initial state and we want to do that, then we're in much deeper trouble, because many services can be mutated, isn't it?

Not so many, actually. The Symfony contributors did a pretty good job on creating stateless non-leaking services. 😎 But yes, there are a few services that carry request-related state that needs to be reset (see #24291).

I would instead expect ppl to not alter services in any way,

Actually, we're the ones disabling the profiler, so we don't profile the profiler and render a debug toolbar inside, you know, the debug toolbar. 😉

symfony/src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php

Line 154 in d6b68e1

$this->profiler->disable();

So, if you revert the re-enabling of the profiler (as you did in your commit), the effect is that you get a debug toolbar on the first request, but not on any subsequent ones handled by the same kernel.

and use reset only for anti-leaking purpose.

To me, reset is meant to be used to purge any (master-)request-related state from services that otherwise would alter the processing on subsequent requests handled by the same kernel.

nicolas-grekas · 2017-10-05T08:25:41Z

Actually, we're the ones disabling the profiler, so we don't profile the profiler and render a debug toolbar inside, you know, the debug toolbar.

Oh cool, that's the piece I was missing :)

Instead of requiring several calls, which is still something I think we should avoid, I propose to add an "$enable" argument to the constructor of Profiler, store it in an $originallyEnabled property (or similar name) and reset to its value on reset().

nicolas-grekas · 2017-10-05T09:22:54Z

@derrabus here is what I have in mind:
nicolas-grekas@2e3be87

derrabus · 2017-10-05T09:36:54Z

Yes, that's what I mentioned in #24289 (comment) already:

If I wanted to do this, the profiler would need to know its initial state (enabled or disabled) so it can reset itself to that state. I don't feel comfortable with saving that information inside the profiler.

But that's merely a matter of taste, I guess. If it's more important to you to not allow multiple reset methods, we can go that way. I'm going to merge your changes now.

nicolas-grekas

Really nice :)

nicolas-grekas · 2017-10-05T09:55:47Z

But tests are failing for now :)

derrabus · 2017-10-05T10:06:55Z

Yeah, you broke them. 😜

nicolas-grekas@10c1691#diff-3c1b20ab38677b0407a30d2c83278e66R60

I'm on it. 😃

derrabus · 2017-10-05T10:34:22Z

Tests are green again. The failure on Travis should be resolved when merging the PR.

nicolas-grekas

Doubling my vote now that tests are green, very happy about that :)

fabpot · 2017-10-05T12:04:30Z

Thank you @derrabus.

This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Reset profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #18244 | License | MIT | Doc PR | N/A This PR adds the ability to reset the profiler between requests. Furthermore, the profiler service has been tagged with the new `kernel.reset` tag from #24155. For this, I had to readd the ability to define multiple reset methods for a service. Note: This PR requires twigphp/Twig#2560. Commits ------- 8c39bf7 Reset profiler.

nicolas-grekas · 2017-10-05T15:27:05Z

PR welcomed on master to remove the legacy code & update the interfaces.

derrabus · 2017-10-06T08:31:41Z

I've already started to work on it.

…abbuh) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] implement reset() in DumpDataCollector | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24289 | License | MIT | Doc PR | Commits ------- cd602d6 implement reset() in DumpDataCollector

derrabus force-pushed the 3.4-reset-profiler branch from 4ac4114 to 55d550c Compare September 22, 2017 17:57

nicolas-grekas requested changes Sep 23, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Sep 24, 2017

derrabus force-pushed the 3.4-reset-profiler branch 5 times, most recently from cacbb30 to d5f0646 Compare September 30, 2017 22:01

derrabus force-pushed the 3.4-reset-profiler branch from d5f0646 to 0bc33fc Compare October 5, 2017 09:48

nicolas-grekas approved these changes Oct 5, 2017

View reviewed changes

carsonbot added the Status: Reviewed label Oct 5, 2017

derrabus force-pushed the 3.4-reset-profiler branch from 0bc33fc to 76fb1bd Compare October 5, 2017 10:09

Reset profiler.

8c39bf7

derrabus force-pushed the 3.4-reset-profiler branch from 76fb1bd to 8c39bf7 Compare October 5, 2017 10:20

nicolas-grekas approved these changes Oct 5, 2017

View reviewed changes

fabpot approved these changes Oct 5, 2017

View reviewed changes

fabpot merged commit 8c39bf7 into symfony:3.4 Oct 5, 2017

derrabus deleted the 3.4-reset-profiler branch October 5, 2017 13:01

xabbuh mentioned this pull request Oct 10, 2017

[HttpKernel] implement reset() in DumpDataCollector #24505

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

marcverney mentioned this pull request Oct 23, 2017

Make RedisDataCollector compatible with Symfony 4.0 snc/SncRedisBundle#365

Closed

fbourigault mentioned this pull request Nov 20, 2017

add reset method to collector Stampie/stampie-bundle#24

Merged

arzurchris mentioned this pull request Oct 16, 2018

Make ProviderDataCollector compatible with Symfony 4 mremi/UrlShortener#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][HttpKernel] Reset profiler #24289

[FrameworkBundle][HttpKernel] Reset profiler #24289

derrabus commented Sep 22, 2017

nicolas-grekas left a comment

nicolas-grekas Sep 23, 2017

derrabus Sep 25, 2017

nicolas-grekas Sep 23, 2017

derrabus Sep 25, 2017

nicolas-grekas Sep 23, 2017

derrabus Sep 25, 2017

nicolas-grekas Sep 29, 2017

nicolas-grekas Sep 23, 2017

derrabus Sep 25, 2017

nicolas-grekas Sep 23, 2017

fabpot Sep 27, 2017

derrabus Sep 28, 2017

derrabus commented Sep 30, 2017

derrabus commented Sep 30, 2017

derrabus commented Sep 30, 2017

nicolas-grekas commented Oct 4, 2017

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 5, 2017

nicolas-grekas commented Oct 5, 2017 •

edited

Loading

nicolas-grekas commented Oct 5, 2017 •

edited

Loading

derrabus commented Oct 5, 2017

nicolas-grekas left a comment

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 5, 2017

derrabus commented Oct 5, 2017

nicolas-grekas left a comment

fabpot commented Oct 5, 2017

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 6, 2017

[FrameworkBundle][HttpKernel] Reset profiler #24289

[FrameworkBundle][HttpKernel] Reset profiler #24289

Conversation

derrabus commented Sep 22, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Sep 30, 2017

derrabus commented Sep 30, 2017

derrabus commented Sep 30, 2017

nicolas-grekas commented Oct 4, 2017

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 5, 2017

nicolas-grekas commented Oct 5, 2017 • edited Loading

nicolas-grekas commented Oct 5, 2017 • edited Loading

derrabus commented Oct 5, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 5, 2017

derrabus commented Oct 5, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Oct 5, 2017

nicolas-grekas commented Oct 5, 2017

derrabus commented Oct 6, 2017

nicolas-grekas commented Oct 5, 2017 •

edited

Loading

nicolas-grekas commented Oct 5, 2017 •

edited

Loading