-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console][FrameworkBundle][HttpKernel][WebProfilerBundle] Enable profiling commands #47416
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
Conversation
23d46cb
to
e87cf77
Compare
Hey! I think @Seb33300 has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
…ms (HeahDude) This PR was merged into the 5.4 branch. Discussion ---------- [WebProfilerBundle] Fix profile search bar link query params | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | ~ | License | MIT | Doc PR | ~ Spotted while working on #47416. The second argument of `controller()` is `attributes`. Commits ------- c09e7a6 [WebProfilerBundle] Fix profile search bar link query params
e87cf77
to
70574d8
Compare
Rebased, this PR is now reviewable. |
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.
Looking forward to playing with it locally
This will be very pleasant for cli based app etc :)
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/layout.html.twig
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/command.html.twig
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
70574d8
to
96a5cdf
Compare
I've pushed a new implementation to use a |
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.
Thanks for this. Here are some comments. I'm not sure about DebugRequestStack
also, this is probably not going to be used elsewhere so I'm leaning toward naming at around the console command use case.
src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php
Outdated
Show resolved
Hide resolved
I've added support for profiling commands interrupted by signals. |
@HeahDude did not find time to test on a personal project it but just a raw comment: given you screenshot in PR desc, is it possible to have the command with its arguments/options being copy on click? ℹ️ also I do not know if it is feasible, if the "server" is symfony cli, copy something like:
instead of
|
This would be very useful. Would a similar abstraction become possible for profiling messages processed by the messenger? |
@Jeroeny as per PR desc |
I think now you can thanks to symfony-cli/symfony-cli#231 |
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.
hi @HeahDude
I find and took some time this morning to test this locally, it is really nice :)
here are some comments mainly in UX/UI usage
also with #47416 (comment) I wrote before
src/Symfony/Bundle/FrameworkBundle/EventListener/ConsoleProfilerListener.php
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/header.html.twig
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/command.html.twig
Outdated
Show resolved
Hide resolved
This is an amazing contribution @HeahDude. I hope this is reviewed and merged soon 🙏 I have some ideas related to the UI of this feature. If you agree, I'd prefer to merge this PR and then I can open a new PR with the proposed UI changes to discuss about them. Thanks! |
Hey, first party CLI profiler support sounds awesome! I have been using a hacky solution to get CLI/Messenger profiles: https://github.com/sourceability/instrumentation/blob/1.0.1/src/Profiler/SymfonyProfiler.php Which is really really useful with https://github.com/sourceability/console-toolbar-bundle that renders the profiler toolbar within the terminal. |
2f5ea2e
to
649e4ef
Compare
649e4ef
to
b404c26
Compare
src/Symfony/Component/HttpKernel/Profiler/ProfilerStorageInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Debug/TraceableEventDispatcher.php
Outdated
Show resolved
Hide resolved
LGTM after a few changes I just pushed, but I agree with @stof it'd be great to get rid of this strange event-dispatcher + listener logic to start/stop sections. Doable in this PR? |
b404c26
to
343a318
Compare
343a318
to
a2081df
Compare
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/command.html.twig
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/header.html.twig
Show resolved
Hide resolved
aeb3266
to
ece480c
Compare
ece480c
to
82914ba
Compare
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.
🚀
Thank you @HeahDude. |
1 and a half year after showing a POC of this feature at the Symfony Live, and 188 comments later, so happy to have this merged. Thanks to all the reviewers for their challenging reviews 🎉 🥰 |
…Dude) This PR was merged into the 7.0 branch. Discussion ---------- [HttpKernel] Remove deprecation layer for Profiler | Q | A | ------------- | --- | Branch? | 7.0 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | ~ | License | MIT Follows #47416. Commits ------- 65970b2 [HttpKernel] Remove deprecation layer for Profiler
…ault) This PR was merged into the 6.4 branch. Discussion ---------- [WebProfilerBundle] Fix generated application link | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT I played a little bit with the new Profiling command #47416 (I love it !), and found that `type=command` is missing in one URL  Commits ------- 477d849 [WebProfilerBundle] Fix generated application link
…javiereguiluz) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [WebProfilerBundle] UI tweaks for the command profiler | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT This PR proposes some minor UI tweaks for the amazing feature contributed by `@HeahDude` in #47416. ## First change I propose to not display the `HTTP / Commands` toggle in the header of all pages. I don't think this should be an option with quick permanent access from all profiler pages. My proposal is to move the toggle to the search sidebar:  And here in action:  ## Second change In my opinion, the current header of command profiles looks too similar to HTTP profiles: ### Before / Light  ### Before / Dark  ----- I propose to differentiate them a bit more and use the well-known "fake terminals" used on Symfony website and docs: ### After / Light  ### After / Dark  ----- The "fake terminals" look different depending on your OS. See this image (from top to bottom: macOS, Windows, Linux)  Commits ------- 041480f [WebProfilerBundle] UI tweaks for the command profiler
{ | ||
$event = $this->stopwatch->start($this->getName().'.execute', 'command'); | ||
|
||
$exitCode = $this->command->execute($input, $output); |
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.
I noticed that this call does not work if the method is defined in a trait.
https://3v4l.org/AFJnQ
This is the case for DoctrineFixturesBundle
with the CommandCompatibility
trait
I don't think it's worth opening an issue.
TLDR;
I've shown a POC of this feature at the Symfony Live Paris last April to some of the core team members (ping @nicolas-grekas, @stof, @lyrixx, @chalasr, @GromNaN).
I propose here a new work from scratch addressing the comments I already got and based on Javier's profiler redesign (#47148).
Reviews should better be done by commits.
Summary
This PR aims to leverage the profiler and its collectors by using a
VirtualRequestStack
to aggregate data on virtual requests.Such requests are obfuscated by default to avoid side effects.
It can feel like a hack... or a pragmatic way to get, without much complexity, tons of useful feedback on what's going on during console execution, from basic info about the command, time/memory metrics, to every existing features already available in HTTP context: events, message dispatching, http requests, emails, serialization, validation, cache, database queries... and so on, all that just out of the box!
Previous work
There were some work to extract the Profiler logic in a dedicated component, that proved to require a lot of complexity and BC breaks in the API:
Screenshots
For now I've focused only on the functional parts.
Search view
Command panel
If the command is signal-able the following panel will be available:

If sub commands are run using

$this->getApplication()->run()
sub profiles will be shown as for requests:The server tab is the same as in the request panel.
Performance panel
Failing command
The exception panel is shown by default as for requests:Sub command
Profile token when verbose
(clickable links with compatible terminals)Command interrupted by signal
Opt-in profiling
Use the new global option
--profile
(in debug only) to profile a command.Future scopes
messenger:consume
with @GromNaN (one of the reasons why I've added anexcludes
option), he told that it would be nice it we could find a way to profile consumers as well.So I've added
an abstractaVirtualRequest
_virtual_type
request attribute and avirtualType
property to profiles, that will allow to create aMessengerWorkerRequest
and a new type of profile with ease in a follow-up PR if the current implementation is accepted.VarDumper
component (/cc @nicolas-grekas)Add a global option in debug to enable/disable the profiler on the fly when running commands (e.g. a negatable--profile
flag)[update] implemented in current scope in replacement of semantic config.
Limitations
No sub profiles are created when using$this->getApplication()->find(...)->run()
because events (needed by the profiler to hook into) are dispatched fromApplication::run()
, not fromCommand::run()
.[update] The docs has been updated in [Console] Improve console events doc symfony-docs#18741.
No profiles are created when killing the command process (i.e. usingctrl-C
, should we add a handler to some signals to force saving profiles?[update] I've added support for this.
Signals as int may not be as useful as they could in the profiler pages, does it worth trying to add a label to some (knowing that some signals can have different constants (labels) with the same int value)?[update] done thanks to [Console] Add
SignalMap
to map signal value to its name #50663.Long running processes should be excluded via configuration to avoid memory leaks[update] profiling is now opt-in using the
--profile
option.messenger:consume
does not work since the kernel is reset after handling a message.TODO
svg for the command panel) /cc @javiereguiluzPR onsymfony/recipes
to add the newframework.profiler.cli
node