Skip to content

[WebProfiler] Add a limit how many profiles should be saved #45831

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

Closed
shyim opened this issue Mar 23, 2022 · 6 comments · Fixed by #47352
Closed

[WebProfiler] Add a limit how many profiles should be saved #45831

shyim opened this issue Mar 23, 2022 · 6 comments · Fixed by #47352

Comments

@shyim
Copy link
Contributor

shyim commented Mar 23, 2022

Description

I let run the profiler for a while and have now 80k profiles saved. It would be nice if we could set a limit of max profiles should be set and delete older ones to save disk

Example

framework:
  profiler:
    max_profiles: 100
@alamirault
Copy link
Contributor

Does anyone know how to implement it ? Is there native/efficient way to manage this ?

(I'm very interested by this feature and can help to the PR)

@fabpot
Copy link
Member

fabpot commented Jul 26, 2022

I think it should be part of FileProfilerStorage (I would not add an interface for that for now as we don't want to support more than one storage anyway).

Instead of a max number of profiles, I would remove the oldest ones, which could be done during a call to write.
I would not make it configurable, though. Profiles are ephemeral by nature. The number of days is debatable, but I don't see any cases where you would have to have a look at a profiler older than a day, to be honest. So, let's say 2 days? Maybe 3 max?

@94noni
Copy link
Contributor

94noni commented Aug 4, 2022

Can we use https://symfony.com/doc/current/reference/events.html#kernel-terminate ?
Defining a DevEvent if the config is present, event responsible to purge old profiles > max_retention_days

Given such config:

framework:
  profiler:
    max_retention_days: 3 # number of days to keep local profile files

@alamirault
Copy link
Contributor

alamirault commented Aug 4, 2022

I think we can implements same logic of monolog RotatingFileHandler in FileProfilerStorage

Have a parameter in config is a good thing

@fabpot
Copy link
Member

fabpot commented Aug 4, 2022

I would keep this as simple as possible, aka not configurable.
Keeping just one day of profiles seems more than enough to me.

@alamirault
Copy link
Contributor

alamirault commented Aug 7, 2022

@fabpot
So when we write a new profile, we want cleaning index file and profiles files.
My approach is to read index file while we have profiles "expired". I assume it's sorted and oldest profile is on firstline.

Is it a good approach ? And cpu/memory efficient implementation ? (seems good when no expired profile, tmp file in other case)

public function removeOldestProfiles(string $indexFilename)
{
$minimalProfileTime = (new \DateTime())
    ->modify(sprintf('-%d days', self::MAX_RETENTION_DAYS))
    ->getTimestamp();

$handle = fopen($indexFilename, 'r');

$tmpIndexFileName = $indexFilename.'.tmp';
$beforeTime = true;
$atLeastOneProfileBeforeTime = false;
while ($beforeTime && $line = fgets($handle)) {
    $csv = str_getcsv($line);
    $profileTime = (int) $csv[4];

    if ($profileTime > $minimalProfileTime) {
        $beforeTime = false;
        file_put_contents($tmpIndexFileName, $line);
    } else {
        $atLeastOneProfileBeforeTime = true;
    }
}

if ($atLeastOneProfileBeforeTime) {
    file_put_contents($tmpIndexFileName, stream_get_contents($handle), FILE_APPEND);
    unlink($indexFilename);
    rename($tmpIndexFileName, $indexFilename);
} else {
    fclose($handle);
}

I can open draft PR if it a better place for theses questions

@fabpot fabpot closed this as completed Dec 18, 2022
fabpot added a commit that referenced this issue Dec 18, 2022
…es mechanism (alamirault)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpKernel] FileProfilerStorage remove expired profiles mechanism

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #45831 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

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:
- Not sure how to deal, profiles with time `0`, like an unexpired profile ?
- I assume profiles are sorted and oldest profile is on firstline. -> avoid readind all index file if first profile is not expired
- Is there a best way ? (cpu/memory, io efficient) (I'm not sure I have the necessary skills, changes are welcome :))

TODOS:

- [x] Changelog
- [x]  Complete tests
- [ ]  Symfony docs PR

Commits
-------

58d0662 [HttpKernel] FileProfilerStorage remove expired profiles mechanism
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants