Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] FileProfilerStorage remove expired profiles mechanism#47352
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
[HttpKernel] FileProfilerStorage remove expired profiles mechanism#47352
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedAug 21, 2022
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
c670f8a to6cd2a51Comparesrc/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
cb75093 tocf10b53CompareUh oh!
There was an error while loading.Please reload this page.
da36747 to600596fCompare
nicolas-grekas 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.
I updated the PR with the following changes:
- set expiration at 2 days
- run randomly 1 / 10
- never write to index.csv but store a seek offset in index.csv.offset
The seek offset allows looking up for files to prune very quickly, that's why 1/10 is enough IMHO.
index.csv is going to grow but this file is small and doing so saves us from race conditions.
alamirault commentedDec 17, 2022
Thanks Nicolas, I like your rework ! |
fabpot commentedDec 18, 2022
Thank you@alamirault. |
162495e to58d0662Compare…age (radar3301)This PR was merged into the 6.3 branch.Discussion----------[HttpKernel] Skip corrupted CSV data in FileProfilerStorage| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | Partiallyfixes#50942, improves/fixes on PR#50913, partially fixes#50816| License | MITTechnically, part of this fix could go all the way back to 2.1, but since we're not sure why/how exactly the index.csv file is being corrupted in the first place, this PR should just serve as a stop-gap measure until the root cause can be identified.A corrupted index.csv file is especially noticeable since 6.3, when the "Remove Expired Profiles" feature was added by#47352 .RE: improves/fixes on PR#50913:Warnings are (usually?) only escalated to errors in debug mode, so having a try/catch is a bit more overhead than I think is actually needed, and I believe just checking that the line read from the csv is not corrupted is the better/faster option. Granted, the profiler should really only be active in debug/non-production modes anyway, but "should" and reality usually don't align...`@alamirault` `@MatTheCat` `@benjaminfunk` `@Pelagoss` `@derrabus`Commits-------b4e942d Update FileProfilerStorage.php
Uh oh!
There was an error while loading.Please reload this page.
This is a first attempt for limit number of profiles saved (discussed in#45831).
When we save new profile, all expired profiles are removed. Expiration is one day for the moment.
Questions:
0, like an unexpired profile ?TODOS: