[FrameworkBundle][HttpKernel][WebProfilerBundle] Enable profiling commands

[HeahDude]

Q	A
Branch?	6.2
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	Fix #45241
License	MIT
Doc PR	~

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 DebugRequestStack 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:

• #10374
• #14809 (see https://github.com/symfony/symfony/pull/14809#issuecomment-313340589)
• #20502

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)

Opt-in config

framework:
    profiler:
        cli:
            enabled: true
            excludes:
                - cache:clear
                - messenger:consume
                - 'some-namespace:'

Future scopes

• When I've discussed the limitation of profiling long running processes such as messenger:consume with @GromNaN (one of the reasons why I've added an excludes option), he told that it would be nice it we could find a way to profile consumers as well. So I've added an abstract VirtualRequest and a virtualType property to profiles, that will allow to create a MessengerWorkerRequest and a new type of profile with ease in a follow-up PR if the current implementation is accepted.
• We could add some dedicated casters for input and output in the VarDumper component (/cc @nicolas-grekas)
• It could be interesting to decorate and collect traces from some helpers (i.e. when running processes)
• Add a global option in debug to enable/disable the profiler on the fly when running commands (e.g. a negatable --profile flag)

Limitations

• No sub profiles are created when using $this->getApplication()->find(...)->run() because events (needed by the profiler to hook into) are dispatched from Application::run(), not from Command::run().
• No profiles are created when killing the command process (i.e. using ctrl-C, should we add a handler to some signals to force saving profiles?
• 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)?
• Long running processes should be excluded via configuration to avoid memory leaks

---

TODO

• [ ] I've left some todos inside the code for reviewers to share they thought before I try going further
• [ ] Add a few tests
• [ ] Get help for the UI (new top nav, svg for the command panel) /cc @javiereguiluz
• [ ] PR on symfony/recipes to add the new framework.profiler.cli node

[carsonbot]

Hey!

I think @Seb33300 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

[HeahDude]

Rebased, this PR is now reviewable.

[HeahDude]

I've pushed a new implementation to use a _virtual request attribute instead of a virtual request type.

[HeahDude]

I've added support for profiling commands interrupted by signals. I've updated the PR description and added a screenshot as well.

[94noni]

@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? like by default the url of the request giving the current profile we are redirected to the url, here we mimic the logic but with the console, thus having the cli il the clipboard

ℹ️ also I do not know if it is feasible, if the "server" is symfony cli, copy something like:

• symfony console app-delete-user -v

instead of

• bin/console app-delete-user -v

[Jeroeny]

This would be very useful. Would a similar abstraction become possible for profiling messages processed by the messenger?

[94noni]

This would be very useful. Would a similar abstraction become possible for profiling messages processed by the messenger?

@Jeroeny as per PR desc Future scopes, I think yes

[maxhelias]

ℹ️ also I do not know if it is feasible, if the "server" is symfony cli, copy something like:

• symfony console app-delete-user -v

instead of

• bin/console app-delete-user -v

I think now you can thanks to https://github.com/symfony-cli/symfony-cli/pull/231

[javiereguiluz]

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!

[adrienbrault]

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. There is a video demo in https://github.com/Sylius/Sylius/pull/12711

[chalasr]

I want this as well. I think targeting 6.4 is fine, we'll need to adapt the way commands are tracked to take the upcoming Console changes into account anyway.

[javiereguiluz]

I'd like this to be one of the main new features of Symfony 7 (more exactly, 6.4 and 7.0).

Please, let's work together to make this mergeable. Later, I'll send a PR to propose some design tweaks so we can iterate on that too. Thanks!

[94noni]

Count me in for another round of review/local testing if needed!

[HeahDude]

@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? like by default the url of the request giving the current profile we are redirected to the url, here we mimic the logic but with the console, thus having the cli il the clipboard

ℹ️ also I do not know if it is feasible, if the "server" is symfony cli, copy something like:

• symfony console app-delete-user -v

instead of

• bin/console app-delete-user -v

---

I've added support for supporting Symfony CLI detection: Copy/pasting the command could be done in another PR, I guess we have enough features in this one to review 😅.

[HeahDude]

The PR is rebased, updated with minor fixes and while getting rid of most limitations from the description which is updated as well.

Following a discussion with @fabpot at the Symfony Live last March, I've dropped the semantic configuration in favor of a global --profile option for opt-in profiling.

The console profiler is still compatible with the only_exceptions config that allows to profile only if the option is set AND if an error occurs, should we drop support for this config as well?

Also, I've leveraged the new SignalMap to display proper labels for handled signals in the profiler page:

The exiting logic of handled signals prevents to profile a command when that occurs because signals listeners are run before the command handle them. So I added a dispatch of console.terminate event in those cases which works pretty well. But I guess it is a feature on its own, maybe to be extracted in another PR, WDYT?

I'm ready for another round of testing and reviews :), thanks!

[fabpot]

The console profiler is still compatible with the only_exceptions config that allows to profile only if the option is set AND if an error occurs, should we drop support for this config as well?

I would drop this :)

[dunglas]

I love this feature, and IMHO the code is close to being good enough for inclusion in 6.4, but relying on a custom "HTTP" request emulation layer is definitely a hack.

In a future iteration, couldn't we make the debug context independent from an HTTP request? For instance, we could introduce a DebugContextInterface that will be implemented by Request as well as by new specialized classes that will not inherit from everything related to HTTP such as CliDebugContext, MessengerDebugContext...).

[fabpot]

What about @dunglas comments?

[HeahDude]

@fabpot all comments addressed, PR rebased, and changelogs updated :).

[chalasr]

As discussed privately, this feature is so useful that it should not have to build on hacks. The doors it opens make it worth being implemented it in a way that does not imply to make CLI commands act as HTTP requests; as it goes against the core design of the Console component.

Basically, we think it's time to reconsider https://github.com/symfony/symfony/pull/14809#issuecomment-313340589 and make the web profiler a standalone component that works in non-HTTP contexts, based on the work started there. Also, that component and its integration should probably be named Inspector instead of WebProfiler.

If we agree then this PR can be closed so that @HeahDude and I can work towards the above goal.

[HeahDude]

Last commit makes DebugRequestStack internal, and removes its type-hint from everywhere. The introduced code of this PR is debug only, all final and internal. We're free to merge it as is in 6.4 and rework it altogether for 7.1 :).

[chalasr]

https://github.com/symfony/symfony/pull/52038 has been merged - rebase unlocked.

[HeahDude]

Rebased and squashed.

[HeahDude]

The « deprecation » label can be removed :).

[nicolas-grekas]

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?

[nicolas-grekas]

Thank you @HeahDude.

[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 🎉 🥰