Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EventDispatcher] Split events across requests #29312

Merged
merged 1 commit into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@ro0NL
Copy link
Contributor

commented Nov 25, 2018

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

Split events per request, as currently a profiled sub-request includes all events. Follows same approach how logs are split in #23659.

@ro0NL ro0NL force-pushed the ro0NL:split-event branch 2 times, most recently from 8d0448d to 924be40 Nov 25, 2018

@ro0NL
Copy link
Contributor Author

left a comment

now called listeners fixed. It's definitely better from the profiler pov.

The UI solves itself btw :) great.

(main request)

image

(sub request)

image

Show resolved Hide resolved src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php
Show resolved Hide resolved src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php Outdated
}
$this->called[$eventName]->attach($listener);
$this->callStack->attach($listener, array($eventName, $this->currentRequestHash));

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 25, 2018

Author Contributor

same issue with keys; grouping by event name breaks the dispatch order

Show resolved Hide resolved src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 25, 2018

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

Last but not least, i think we should append each wrapped listener to the call stack during pre process and lazy verify if it was called yes/no.

Currently it's added to the call stack during post-process which affects the order. I.e. the main kernel.exception is called before the sub kernel.reques, but we only collect that after the sub request finished.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

Ive updated above screenshots after c737944

@ro0NL ro0NL force-pushed the ro0NL:split-event branch from a2a320e to 5d9a2e2 Nov 25, 2018

@ro0NL ro0NL changed the title [EventDispatcher][WIP] Split events across requests [EventDispatcher] Split events across requests Nov 25, 2018

@ro0NL ro0NL force-pushed the ro0NL:split-event branch from 5d9a2e2 to 04ddd05 Nov 25, 2018

nicolas-grekas added a commit that referenced this pull request Dec 1, 2018

minor #29397 [WebProfilerBundle] Fix title case (ro0NL)
This PR was merged into the 4.1 branch.

Discussion
----------

[WebProfilerBundle] Fix title case

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Split from #29312 for 4.1 👼

Commits
-------

3e16e25 [WebProfilerBundle] Fix title case

nicolas-grekas added a commit that referenced this pull request Dec 17, 2018

bug #29411 [EventDispatcher] Revers event tracing order (ro0NL)
This PR was merged into the 3.4 branch.

Discussion
----------

[EventDispatcher] Revers event tracing order

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Split from #29312 for 3.4

This traces events in dispatch order.

Before:

![image](https://user-images.githubusercontent.com/1047696/49330956-5f71e780-f596-11e8-8701-80458e715213.png)

After:

![image](https://user-images.githubusercontent.com/1047696/49330963-79abc580-f596-11e8-8666-5535afd59b31.png)

Here we see it also collects "terminate" events (both kernel & console for not-called listeners). In case of exception page the fix is even better: #29312 (review)

Which now correctly shows the kernel.exception event of the main request is dispatched _before_ the kernel.request event of the sub-request.

Moreover, it de-duplicates events. So we actually see the sub-request events 🎉

_Havent looked at tests yet._

Commits
-------

2570d6f [EventDispatcher] Revers event tracing order

symfony-splitter pushed a commit to symfony/event-dispatcher that referenced this pull request Dec 17, 2018

bug #29411 [EventDispatcher] Revers event tracing order (ro0NL)
This PR was merged into the 3.4 branch.

Discussion
----------

[EventDispatcher] Revers event tracing order

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Split from #29312 for 3.4

This traces events in dispatch order.

Before:

![image](https://user-images.githubusercontent.com/1047696/49330956-5f71e780-f596-11e8-8701-80458e715213.png)

After:

![image](https://user-images.githubusercontent.com/1047696/49330963-79abc580-f596-11e8-8666-5535afd59b31.png)

Here we see it also collects "terminate" events (both kernel & console for not-called listeners). In case of exception page the fix is even better: symfony/symfony#29312 (review)

Which now correctly shows the kernel.exception event of the main request is dispatched _before_ the kernel.request event of the sub-request.

Moreover, it de-duplicates events. So we actually see the sub-request events 🎉

_Havent looked at tests yet._

Commits
-------

2570d6f877 [EventDispatcher] Revers event tracing order
@nicolas-grekas
Copy link
Member

left a comment

What about making the class @final? (if you agree, don't forget updating the changelog file)

  • rebase needed btw
@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

@ro0NL Do you think you can finish this one for 4.3?

@ro0NL ro0NL force-pushed the ro0NL:split-event branch 4 times, most recently from 7c9f24e to 5bcd163 Apr 2, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@fabpot done.

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@ro0NL Apparently, tests do not pass. Can you have a look please?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Let's see now :) i think it's a shortcoming of sf/debug not accounting for @inheritdoc :/

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

I think it's a shortcoming of sf/debug not accounting for @inheritdoc :/

It's not a bug it's a feature. You must have to change your code when a lower layer decides to deprecate something. This is how.

@fabpot

fabpot approved these changes Apr 2, 2019

@fabpot fabpot force-pushed the ro0NL:split-event branch from ac242f9 to c3477ba Apr 2, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit c3477ba into symfony:master Apr 2, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 2, 2019

feature #29312 [EventDispatcher] Split events across requests (ro0NL)
This PR was squashed before being merged into the 4.3-dev branch (closes #29312).

Discussion
----------

[EventDispatcher] Split events across requests

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #24275
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Split events per request, as currently a profiled sub-request includes all events. Follows same approach how logs are split in #23659.

Commits
-------

c3477ba [EventDispatcher] Split events across requests

@ro0NL ro0NL deleted the ro0NL:split-event branch Apr 2, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.