Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Persist app bootstrapping logs for logger datacollector#21502

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

Merged
fabpot merged 2 commits intosymfony:masterfromScullWM:container_build
Apr 20, 2017

Conversation

@ScullWM
Copy link
Contributor

@ScullWMScullWM commentedFeb 1, 2017•
edited by nicolas-grekas
Loading

QA
Branch?3.3
Bug fix?no
New feature?yes
BC breaks??
Deprecations?no
Tests pass?yes
Fixed tickets#21405
LicenseMIT

Logs generated during the container build are catched by the BufferingLogger with a special flag.

They are persist by the LoggerDataCollector and are available in the logger profiler.
In the profiler toolbar, the "container build" logs increment the current logs counter (even if the container was previously built).

capture d ecran 2017-02-01 a 20 56 40

capture d ecran 2017-02-01 a 20 57 55

The BufferingLogger now require the cachePath and the filesystem to persist a (unique) container build logs.
If the current workflow is ok, I will update the test coverage (actually they fail). Maybe we can display the appDevDebugProjectContainerCompiler.log content in that logger profile.

fbourigault and Koc reacted with heart emoji

private$containerBuildLogs = [];

publicfunction__construct(Filesystem$filesystem,$logger =null,$cacheDir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this cannot be required. It is a BC break.

And adding a new argument at the beginning is even worse.


publicfunctionpersistContainerBuildLogs()
{
$datas = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

no short array syntax please

if ($this->isSilencedOrDeprecationErrorLog($log)) {
if ($log['context']['exception']instanceof SilencedErrorContext) {
++$count['scream_count'];
}elseif (isset($log['context'][BufferingLogger::BOOTSTRAPPING_FLAG])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is adding a hard dependency to the Debug component to read the constant

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There is already a dependency to the debug component on theSilencedErrorContext. I understand that it create a (second) hard dependency between those components, a hard coded key would be better?

$datas =unserialize($rawContent);

$this->data['container_build_count'] =$datas['container_logs'];
$this->data['container_build_logs'] =$this->sanitizeLogs($datas['logs']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sanitizing must be done before dumping. Otherwise, you have no guarantee that the logs will be serializable (the context can contain objects)

@stof
Copy link
Member

stof commentedFeb 1, 2017

@nicolas-grekas I'm not sure about this approach. Can you check it ?

@stof
Copy link
Member

stof commentedFeb 1, 2017

Naming the tabcontainer logs looks weird to me, as you are not storing the logs of the container builder, but the errors being logged during the initialization phase.

@ScullWM
Copy link
ContributorAuthor

App Bootstrapping could be a better name., in fact that's exactly what's done.

@ScullWMScullWM changed the titlePersist container building logs for logger datacollectorPersist app bootstrapping logs for logger datacollectorFeb 2, 2017
@nicolas-grekasnicolas-grekas added this to the3.x milestoneFeb 2, 2017
@ScullWMScullWMforce-pushed thecontainer_build branch 3 times, most recently from47d826d toa329dcfCompareFebruary 2, 2017 20:01
@fabpot
Copy link
Member

ping@nicolas-grekas

@ScullWM
Copy link
ContributorAuthor

ScullWM commentedMar 7, 2017•
edited
Loading

Sorry but recents updates on master have interactions with this PR and it look like it's not working anymore.
Investigating on it...

Edit:
Here we are:
First way to do that was to flag handled logs before monolog is active.

My bad idea was to think that if there are some flagged logs, it mean that we are bootstrapping the app.

But in some case, thevar/cache/dev/classes.php will always trigger logs (flagged). Causing different number of logs than the previous request, and so, meaning that we have a new app bootstrapping.

Difficulty is to detect if a new application bootstrapping have been made from theLoggerDataCollector without the kernel (or creating a new DataCollector).

Investigating on it...

@ScullWM
Copy link
ContributorAuthor

ScullWM commentedMar 22, 2017•
edited
Loading

I've updated my PR using aBootstrappingLoggerLoader.

The idea is to associate the "app bootstrapping logs" with the container built using the md5_file.

This way, it display at every request, the handled logs before monolog is active (called app bootstrapping logs).

capture d ecran 2017-03-29 a 13 53 44

Samffy reacted with thumbs up emoji

@ScullWMScullWMforce-pushed thecontainer_build branch 11 times, most recently fromd186274 to69fd3feCompareApril 5, 2017 19:53
@ScullWM
Copy link
ContributorAuthor

ScullWM commentedApr 5, 2017•
edited
Loading

I've updated this PR to implement full tests ofBootstrappingLoggerLoader.

I hope it will help other devs to update easily their Symfony version (finding deprecation and logs who rarely happen).

@nicolas-grekas
Copy link
Member

can you please rebase against master to remove merge commits in the current git history in this PR?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Last comment I guess :)
Looking at the screenshot, I think only the file + line info is usefull - not the full context array (scream & co are useless, message is duplication)


$containerDeprecationLogs =$this->getContainerDeprecationLogs();
$this->data['deprecation_count'] +=count($containerDeprecationLogs);
$this->data['container_compiler_logs'] =$this->getContainerCompilerLogs();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

let's rename this to compiler_logs to be consistent with the public methodgetCompilerLogs

ScullWM reacted with thumbs up emoji
'line' =>$line,
);

return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should be removed

ScullWM reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

Also, the "Container Compilation" count needs a fix

@nicolas-grekasnicolas-grekas self-requested a reviewApril 17, 2017 13:31
@nicolas-grekasnicolas-grekasforce-pushed thecontainer_build branch 2 times, most recently from9a58cf1 to0d4b75fCompareApril 17, 2017 13:41
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 17, 2017•
edited
Loading

@ScullWM FYI I pushed on your branch on your fork, adding a 2nd commit + tweaking yours a bit to fine tune DX.

Here is a screenshot, showing:

  • trace excerpts are collapsed by default now
  • boot time deprecations have their file+line dumped trace-like
  • parts of log messages between double-quotes are put in bold (all panels)
  • paths are dumped with ellipsed prefixes, and this prefix is now computed based on vendor+composer.json location
  • ellipsis have a "tail" section, see "twig/twig" in the screenshot for what it is

capture du 2017-04-17 18-48-24

capture du 2017-04-17 15-45-36

@nicolas-grekasnicolas-grekasforce-pushed thecontainer_build branch 6 times, most recently fromb719e10 to99442edCompareApril 17, 2017 15:08
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

@nicolas-grekas
Copy link
Member

(fabbot failures are false-positives in this case)

@nicolas-grekasnicolas-grekas modified the milestones:3.3,3.4Apr 18, 2017
@fabpot
Copy link
Member

Thank you@ScullWM.

@fabpotfabpot merged commit2fd18b5 intosymfony:masterApr 20, 2017
fabpot added a commit that referenced this pull requestApr 20, 2017
…r (ScullWM, nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------Persist app bootstrapping logs for logger datacollector| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | no| New feature?  | yes| BC breaks?    | ?| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21405| License       | MITLogs generated during the container build are catched by the BufferingLogger with a special flag.They are persist by the LoggerDataCollector and are available in the logger profiler.In the profiler toolbar, the "container build" logs increment the current logs counter (even if the container was previously built).<img width="540" alt="capture d ecran 2017-02-01 a 20 56 40" src="https://cloud.githubusercontent.com/assets/1017746/22523826/0bc12e4a-e8c1-11e6-830f-7f6238ea7423.png"><img width="1022" alt="capture d ecran 2017-02-01 a 20 57 55" src="https://cloud.githubusercontent.com/assets/1017746/22523859/2c48a698-e8c1-11e6-9bdb-d85f3e692938.png">The BufferingLogger now require the cachePath and the filesystem to persist a (unique) container build logs.If the current workflow is ok, I will update the test coverage (actually they fail). Maybe we can display the appDevDebugProjectContainerCompiler.log content in that logger profile.Commits-------2fd18b5 [VarDumper] Fine tune dumping log messagesce3ef6a Persist app bootstrapping logs for logger datacollector
@fabpotfabpot mentioned this pull requestMay 1, 2017
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull requestMar 25, 2018
…ollector (ScullWM, nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------Persist app bootstrapping logs for logger datacollector| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | no| New feature?  | yes| BC breaks?    | ?| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony#21405| License       | MITLogs generated during the container build are catched by the BufferingLogger with a special flag.They are persist by the LoggerDataCollector and are available in the logger profiler.In the profiler toolbar, the "container build" logs increment the current logs counter (even if the container was previously built).<img width="540" alt="capture d ecran 2017-02-01 a 20 56 40" src="https://cloud.githubusercontent.com/assets/1017746/22523826/0bc12e4a-e8c1-11e6-830f-7f6238ea7423.png"><img width="1022" alt="capture d ecran 2017-02-01 a 20 57 55" src="https://cloud.githubusercontent.com/assets/1017746/22523859/2c48a698-e8c1-11e6-9bdb-d85f3e692938.png">The BufferingLogger now require the cachePath and the filesystem to persist a (unique) container build logs.If the current workflow is ok, I will update the test coverage (actually they fail). Maybe we can display the appDevDebugProjectContainerCompiler.log content in that logger profile.Commits-------2fd18b5 [VarDumper] Fine tune dumping log messagesce3ef6a Persist app bootstrapping logs for logger datacollector
nicolas-grekas added a commit that referenced this pull requestJun 19, 2018
This PR was merged into the 4.2-dev branch.Discussion----------CacheWarmerAggregate handle deprecations logs| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#27387| License       | MITActually the Web Debug Toolbar warning you about the deprecation messages thrown during the container built (#21502).Cache warmup can throw deprecated message without any persistance, it may cause issue like#27387This PR reproduce the same job for the cache warmer, and so on, handle deprecated messages during the warmup of Twig, Translator, Validator, Security and all `kernel.cache_warmer` services.Here are the point that may be improvable in this PR:1.  Actually I've "duplicate" the callable used in the `set_error_handler` of the Kernel.IMHO I think that Kernel and CacheWarmerAggregate have differents jobs and a trait may be a good solution to share this error handler setter without duplicating the code, but I'm a little bit lost about the repercussion of adding a Trait in the Kernel.2. I've think about extending the `CacheWarmerAggregate` into a `DeprecatedLogHandlingCacheWarmerAggregate` to add the debug and containerClass argument, and declare it as the `cache_warmer` service only in debug mode (by declaring it in the DebugBundle/Resources/config/services.xml).Commits-------f03b8bb CacheWarmerAggregate handle deprecations logs
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

5 participants

@ScullWM@stof@fabpot@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp