Remove model name sub-collision logging #2214
Conversation
|
Ideally I'd have liked to log the number of times its being reused, but the current testing is a bit shallow on capabilities. I could just hardcode the "is being used 2 times." into the test and it'd pass phpunit but that is a bit misleading since we're only testing a collision depth of 1... Then again, current testing doesn't take secondary collisions into account so I guess both ways are a tad misleading... Sorry for the mention, but as the person leading the recent surge of maintenance, I figured your input would be valuable- Do you have any thoughts @DjordyKoert |
I don't think it hurts to rewrite the current test to ensure this doesn't happen again. |
Since #2131 it's possible to pass a |
That actually mostly fixes my issue. Can't believe I missed that getting added! |
I stand corrected- it appears that the Symfony Profiler captures all logged items regardless of whether a nullhandler is used to "blackhole" them or not. My Symfony Profiler still OOMs the browser when trying to view the logs for the /doc page. As a bit of an experiment, I set monolog to separate the Nelmio logs into their own file- it generates a ~290mb logfile of name collisions for 1 /doc request. With the changes in this PR, the logfile generated for the /doc page is only ~4mb and my browser doesn't crash. I'd call that a good start. |
|
I am happy to merge this if you could write some tests for this (probably by rewriting the current test). |
Model/ModelRegistry.php
Outdated
|
|
||
| if ($i > 1) { | ||
| if (isset($this->registeredModelNames[$base])) { | ||
| $this->logger->info(sprintf('Can not assign a name for the model, the name "%s" has already been taken.', $base), [ |
There was a problem hiding this comment.
| $this->logger->info(sprintf('Can not assign a name for the model, the name "%s" has already been taken.', $base), [ | |
| $this->logger->info(sprintf('Can not assign a name for the model, the name "%s" has been taken %d times.', $base, $i), [ |
|
@emhovis could you rebase with last updates? Thanks |
Things are slowing down a bit for me, I'll see if I can set some time aside soon for writing those tests so we can finally get this merged |
With all of the love this bundle has recently been getting, I got tired of manually removing the logging functionality every update to be able to actually use my local environment's OA docs.
This should alleviate a good chunk of the problems referenced in #2099
Rather than logging ~n^2/2 average times per shared name, this change cuts it down to at most once per shared name. There's still quite a bit of logging, but this cuts down the number of logs by an order of magnitude... so I'd say its a good start. If you have 1000 models named "image", and load the entire doc reference, the current package would generate ~500k logs but with the changes in this PR would only generate 999.
I'd prefer if it were an option we could opt out of logging it in its entirety via a config var but I've spent far too long trying to makeshift that to work.