Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Improve the profiler panel#27202
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
yceruto commentedMay 8, 2018
+1 for segregate by bus using tabs. |
sroze commentedMay 8, 2018
👍 to keep it by call order. It makes much more sense when you want to investigate what happened (which is the point of the profiler) |
ogizanagi commentedMay 8, 2018
@sroze : Then what about a default "All" tab ? :) |
sroze commentedMay 8, 2018
Maybe we can have a collapsed/expanded version? The collapsed will just be the bus name and the message name. You can click on it and it displays this tabled version? |
ro0NL commentedMay 9, 2018
What about a filter and perhaps share some logic with#24263? |
ogizanagi commentedMay 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
New version pushed & screens updated in PR description. @sroze : A simple collapsible version is indeed better. Long messages' FQCN wouldn't suit great for what I had in mind at first, and the amount of messages per request would never be big, so it's a great idea! @ro0NL : when#24263 is merged, let's see what we can do here. For now, tabs are perfect to me :) |
| */ | ||
| class TraceableMessageBusimplements MessageBusInterface | ||
| { | ||
| privatestatic$calls =0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This might not be ideal. Use a microtime to keep calls order instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's wrong with keeping PHP incrementing the keys with$this->dispatchedMessages[]?
ogizanagiMay 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are multiple buses, so multiple traceable buses. This ensures the calls order is kept accross all instances, while iterating over each traceable buses in the datacollector to get their dispatched messages separately won't 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yeah, obviously. 🤦♂️ . What about adding thetime (asmicrotime(true)) in thedispatchedMessage object andusorting them when displaying or late collecting them? (using it as the key might - very very unlikely but still - colide)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's what I suggested in#27202 (comment) :p
I take that as a "Let's do it" then 😄 Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🙊
yceruto commentedMay 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This looks great! just a minor comment, making bg-red each counter with exception is a bit confusing at first sight, is there other way to represent that the tab has exceptions? |
ogizanagi commentedMay 10, 2018
Not sure. Actually that's kind of what we're already used to when there is an "error". For instance any item in the toolbar or the menu. |
yceruto commentedMay 10, 2018
For example, could we show a |
ogizanagi commentedMay 10, 2018
Any opinion? I think I like it :) |
yceruto commentedMay 10, 2018
Another sample of the same: |
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That looks amazing ❤️
| */ | ||
| class TraceableMessageBusimplements MessageBusInterface | ||
| { | ||
| privatestatic$calls =0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's wrong with keeping PHP incrementing the keys with$this->dispatchedMessages[]?
mvrhov commentedMay 11, 2018
IMO "All buses" could easily be shortened to just "All" |
sroze commentedMay 12, 2018
The tests are failing now by the way :) |
ogizanagi commentedMay 12, 2018
@sroze: I know :) I'll try to make this ready this afternoon. |
sroze commentedMay 12, 2018 via email
No worries, no pressure at all. …On Sat, May 12, 2018 at 11:01 AM Maxime Steinhausser < ***@***.***> wrote:@sroze <https://github.com/sroze>: I know :) I'll try to make this ready this afternoon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27202 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEda0mGEnSi02Wd6T9DDCWKWgiukQks5txqTZgaJpZM4T3Agu> . |
ogizanagi commentedMay 12, 2018
Ready I've also changed "All buses" to "All" as suggested by@mvrhov and added a small informational note below tabs. |
yceruto commentedMay 12, 2018
@ogizanagi what about don't show the "Exception" row if "No Exception"? |
ogizanagi commentedMay 12, 2018
Done :) |
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Amazing work. Thank you!
| <h3class="tab-title">{{bus }}<spanclass="badge {{exceptionsCount?exceptionsCount==messages|length?'status-error':'status-some-errors' }}">{{messages|length }}</span></h3> | ||
| <divclass="tab-content"> | ||
| <pclass="text-muted">Ordered list of messages dispatched by the <code>{{bus }}</code> bus</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
...dispatch *on* the ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed!
fabpot commentedMay 13, 2018
Thank you@ogizanagi. |
This PR was squashed before being merged into the 4.1 branch (closes#27202).Discussion----------[Messenger] Improve the profiler panel| Q | A| ------------- | ---| Branch? | 4.1 <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!--#26597 issue number(s), if any -->| License | MIT| Doc PR | N/AThis is an attempt to enhance the profiler panel a bit.**with the following messages dispatched:**```php$queryBus->dispatch(Envelope::wrap(new GetGreetingsQuery('Hello you')) ->with(new JustAFriendOfMine()) ->with(new AndHisPlusO̶n̶e̶Eleven()));$commandBus->dispatch(new SendGiftCommand());$queryBus->dispatch(new GetGreetingsQuery('Exterminate!'));```## Before<img width="1084" alt="screenshot 2018-05-12 a 13 57 57" src="https://user-images.githubusercontent.com/2211145/39957055-8a0f009e-55ec-11e8-9d8e-bf79aad4b420.PNG">🐛calls order are wrong here, fixed in this PR## After### collapsed<!-- <img width="1083" alt="screenshot 2018-05-10 a 23 51 07" src="https://user-images.githubusercontent.com/2211145/39896093-19a8c7ee-54ad-11e8-8dcb-4e165ffd2eae.PNG">--><img width="1085" alt="screenshot 2018-05-12 a 13 18 35" src="https://user-images.githubusercontent.com/2211145/39956802-9d4c38a2-55e7-11e8-8425-ad090c0871b6.PNG"><img width="1085" alt="screenshot 2018-05-12 a 13 26 44" src="https://user-images.githubusercontent.com/2211145/39956827-25d9e426-55e8-11e8-9116-160603649f33.PNG">📝 _When loading the page, all messages details are collapsed by default but the first one per tab._### expanded<!-- <img width="1083" alt="screenshot 2018-05-10 a 23 13 39" src="https://user-images.githubusercontent.com/2211145/39894779-42c9cc9a-54a8-11e8-9529-6292481536d4.PNG"> --><img width="1086" alt="screenshot 2018-05-12 a 13 49 42" src="https://user-images.githubusercontent.com/2211145/39956981-639fc3d6-55eb-11e8-9224-a48f591db3da.PNG">### live<!--  -->### toolbar (with exceptions)<img width="284" alt="screenshot 2018-05-10 a 23 18 32" src="https://user-images.githubusercontent.com/2211145/39895011-0467f2a0-54a9-11e8-9d78-25461cf71c41.PNG">## Notes- Table headers are clickable, so you can jump directly to the message class in the code- Reversing headers/rows allows to have a wider space for dumps and allows to add more entries in the future. This is an issue we already have with the Validator panel (when there is both an invalid value as object and a constraint violation dumped) which I'd like to revamp soon.- ~~I wonder if we should keep the dispatched messages in call order, or if we can segregate by bus (using tabs?).~~- ~~we could add a left container listing messages classes only, allowing to show details of a single message dispatched on a right container (similar to what the Form panel does). I'll probably suggest the same for the Validator panel.~~Commits-------3d19578 [Messenger] Improve the profiler panel
Uh oh!
There was an error while loading.Please reload this page.
This is an attempt to enhance the profiler panel a bit.
with the following messages dispatched:
Before
🐛calls order are wrong here, fixed in this PR
After
collapsed
📝When loading the page, all messages details are collapsed by default but the first one per tab.
expanded
live
toolbar (with exceptions)
Notes
I wonder if we should keep the dispatched messages in call order, or if we can segregate by bus (using tabs?).we could add a left container listing messages classes only, allowing to show details of a single message dispatched on a right container (similar to what the Form panel does). I'll probably suggest the same for the Validator panel.