Skip to content

[Cache][EventDispatcher][HttpClient][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is enabled #60243

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

Merged
merged 1 commit into from
Apr 27, 2025

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Apr 18, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues #60037
License MIT

This PR ensures traceable decorators are enabled only if the profiler is also enabled.

The way it works is by adding a new argument to all traceable decorators: ?\Closure $disabled. When a closure is given and when the closure returns true, tracing should be skipped. Calling the closure should be done before every decorated method to cope for dynamically enabled/disabled states.

Then, this argument is wired using dependency injection so that a closure is passed that returns the current state of the profiler. If the profiler is not instantiated yet (eg because we're on the CLI, or because we're before ProfilerListener enabled it), then a default state is used (disabled on the CLI, enabled on the web). This allows collecting data on the web even if the profiler didn't start yet (as happens when a request comes in, before ProfilerListener is triggered).

Note that I didn't change all Traceable* decorators: the remaining ones look inoffensive to me. Of course, this could be wrong and fixed in follow up PRs.

@carsonbot carsonbot added this to the 7.3 milestone Apr 18, 2025
@nicolas-grekas nicolas-grekas requested a review from lyrixx as a code owner April 18, 2025 13:35
@carsonbot carsonbot changed the title [Cache] Don't enable cache tracing unless the profiler is enabled [Cache][EventDispatcher][HttpKernel][Validator][Workflow] Don't enable cache tracing unless the profiler is enabled Apr 18, 2025
@carsonbot carsonbot changed the title [Cache][EventDispatcher][HttpKernel][Validator][Workflow] Don't enable cache tracing unless the profiler is enabled [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable cache tracing unless the profiler is enabled Apr 18, 2025
lyrixx

This comment was marked as outdated.

@nicolas-grekas

This comment was marked as outdated.

@stof

This comment was marked as outdated.

@nicolas-grekas

This comment was marked as outdated.

@nicolas-grekas nicolas-grekas force-pushed the c-trace branch 2 times, most recently from 02cd114 to 4c2f79a Compare April 18, 2025 14:19
@nicolas-grekas nicolas-grekas changed the title [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable cache tracing unless the profiler is enabled [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is enabled Apr 18, 2025
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Apr 18, 2025

Except using global state and hardcoding, I don't see how one could make each tracing decorator know about the profiler - they're decoupled.

OK, here is a way: make the added $enabled properties bool|Closure():bool, then make all tracing decorators call the closure everytime there's a need to fetch the current state. Then generate this closure using DI and some bridge class between the profiler's state and the bool that we need.

Let's try the current simpler approach first and improve later, if the need arises.

@nicolas-grekas nicolas-grekas changed the title [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is enabled [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is instantiated Apr 18, 2025
@nicolas-grekas nicolas-grekas force-pushed the c-trace branch 2 times, most recently from 5781dc7 to bd0a99f Compare April 18, 2025 16:34
@GromNaN
Copy link
Member

GromNaN commented Apr 18, 2025

The Closure would be $profiler->isEnabled(...). This could create a circular reference.

@nicolas-grekas
Copy link
Member Author

The Closure would be $profiler->isEnabled(...). This could create a circular reference.

Basically yes, but I'd do it in a more subtle way: I'd add a intermediary closure who would get the profiler using a scoped container, and this container would reference the profiler using IGNORE_ON_UNINITIALIZED_REFERENCE, so that the profiler is not instantiated on this side of the relation.

@carsonbot carsonbot changed the title [Cache][EventDispatcher][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is instantiated [Cache][EventDispatcher][HttpClient][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is instantiated Apr 22, 2025
@nicolas-grekas nicolas-grekas changed the title [Cache][EventDispatcher][HttpClient][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is instantiated [Cache][EventDispatcher][HttpClient][HttpKernel][Messenger][Validator][Workflow] Don't enable tracing unless the profiler is enabled Apr 22, 2025
@nicolas-grekas nicolas-grekas force-pushed the c-trace branch 2 times, most recently from bd0d686 to 37412d9 Compare April 22, 2025 09:08
@nicolas-grekas
Copy link
Member Author

PR updated with the closure I described in my previous comment: I figured out it was actually simpler this way, because it then doesn't need patching data collectors anymore.

@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Apr 24, 2025
@94noni
Copy link
Contributor

94noni commented Apr 25, 2025

@nicolas-grekas according to #60037 (comment)

In devland code (external bundles or app « tracers »)

What are the changes required to mimic this?
only inject such new service and check the state prior to code exec? If so cant this be autowired in some way or not?

Is it worth an entry in the doc as well ? (Perhaps near data collector section as most likely to be related)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Apr 25, 2025

Autowiring that should be possible with some named alias. I'm not going to be able to update this PR soon but it could be merged and followed up by the alias one.
Doc indeed!

@94noni
Copy link
Contributor

94noni commented Apr 26, 2025

Feel free to ping me for doc part :)

@fabpot
Copy link
Member

fabpot commented Apr 27, 2025

Thank you @nicolas-grekas.

@fabpot fabpot merged commit f80550e into symfony:7.3 Apr 27, 2025
4 of 11 checks passed
@nicolas-grekas nicolas-grekas deleted the c-trace branch May 19, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants