Skip to content
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

[5.4] Add log service provider, defers loading of logger #15451

Merged
merged 1 commit into from Oct 6, 2016

Conversation

garygreen
Copy link
Contributor

@garygreen garygreen commented Sep 15, 2016

This re-adds the LogServiceProvider back to master which was originally how it was done in 4.2.

There is a major benefit in deferring the initialisation of the logger from the bootstrapping process -- it prevents it from being instantiated on every single request. Instead, it will only get loaded when there is an exception or something generally needs to be logged.


Currently these files are included in every request:

191 files, 105ms, 5.36MB

...
...
vendor\laravel\framework\src\Illuminate\Foundation\Bootstrap\ConfigureLogging.php
vendor\laravel\framework\src\Illuminate\Log\Writer.php
vendor\laravel\framework\src\Illuminate\Contracts\Logging\Log.php
vendor\psr\log\Psr\Log\LoggerInterface.php
vendor\monolog\monolog\src\Monolog\Logger.php
vendor\monolog\monolog\src\Monolog\Handler\StreamHandler.php
vendor\monolog\monolog\src\Monolog\Handler\AbstractProcessingHandler.php
vendor\monolog\monolog\src\Monolog\Handler\AbstractHandler.php
vendor\monolog\monolog\src\Monolog\Handler\HandlerInterface.php
vendor\monolog\monolog\src\Monolog\Formatter\LineFormatter.php
vendor\monolog\monolog\src\Monolog\Formatter\NormalizerFormatter.php
vendor\monolog\monolog\src\Monolog\Formatter\FormatterInterface.php
...
...

After change:

180 files, 99ms, 5.09MB (6% lower)
...
...
vendor\laravel\framework\src\Illuminate\Log\LogServiceProvider.php
...
...

@GrahamCampbell GrahamCampbell changed the title Add log service provider, defers loading of logger. [5.4] Add log service provider, defers loading of logger Sep 16, 2016
@GrahamCampbell
Copy link
Member

Not sure this is a good idea. The logger is lightweight and it's important that it's available as early as possible to the framework.

@garygreen
Copy link
Contributor Author

garygreen commented Sep 16, 2016

True, it does need to be early as possible so maybe it could be registered earlier - however I do think it needs to be deferred in some way. Initialising the logger on every single request just doesn't seem necessary.

I did some tests and found that after deferring the logger, it removes 11 files from the bootstrapping process of your app and memory usage goes down by 6%.

@garygreen
Copy link
Contributor Author

I think I may send this to 5.3, we can initialise the service provider from within Application.php registerBaseServiceProviders() this gets executed before any of the bootstrapping process, it'll be backwards compatible and it'll technically mean the logger is available even faster than it is now. What do you think @GrahamCampbell ?

@GrahamCampbell
Copy link
Member

No way this could go to 5.3. I'm reluctant to see this change at all. It's definitely breaking for me.

@garygreen
Copy link
Contributor Author

garygreen commented Sep 16, 2016

Ok I'll leave it in master. Curious as to why you think this would be breaking exactly? If it was registered in registerBaseServiceProviders() it wouldn't be breaking for anyone. It could even be registered in the ConfigureLogging itself.

@taylorotwell
Copy link
Member

It seems to work in general for me. Would be curious to here more of Graham's concerns.

@garygreen
Copy link
Contributor Author

garygreen commented Sep 21, 2016

@taylorotwell I've updated the PR so the service provider is registered as part of the core ones (and added it to compiled.php so it's always loaded), that way it doesn't need to be added to the app providers. Technically the logger is now available even sooner than it is at the moment. There doesn't appear to be any disadvantages to this PR, unless Graham can highlight otherwise?

@taylorotwell
Copy link
Member

How is it going to be available sooner if you are using app()->environment() and DetectEnvironment is a bootstrapper?

@garygreen
Copy link
Contributor Author

garygreen commented Oct 6, 2016

How is it going to be available sooner if you are using app()->environment() and DetectEnvironment is a bootstrapper?

Ah true. It's not available any sooner or later than it is it now, though you get the benefit of not loading all the logger files on every request.

@taylorotwell taylorotwell merged commit 919f21b into laravel:master Oct 6, 2016
@taylorotwell
Copy link
Member

I made some additional tweaks to this so that the logger is immediately usable once the LogServiceProvider has been registered. If the environment hasn't been loaded yet it will default to production. If the configuration hasn't been loaded it will default to the "single" file logger. So that at least some usable log is generated if you have a log fire between when the logger is registered and the bootstrappers run.

@garygreen
Copy link
Contributor Author

Thanks @taylorotwell. Looks good! 😄

@garygreen garygreen deleted the log-service-provider branch October 20, 2016 19:02
@jeffersonsetiawan
Copy link
Contributor

jeffersonsetiawan commented Feb 10, 2017

With this change, I can't use Log to file and Bugsnag (Laravel bugsnag) at the same time.

If I registered the bugsnag alias (bugsnag.logger), I can't write Log to file (storage/logs).

@garygreen can you help me to troubleshooting the Log in bugsnag? need help.

Thanks.

@GrahamCampbell
Copy link
Member

Where is you code for Bugsnag. Is it in the boot method?

@jeffersonsetiawan
Copy link
Contributor

Fix here

Just have to change bugsnag.logger to bugsnag.multi.
I put it on the boot method on AppServiceProvider.
@GrahamCampbell I create another provider for log aka LogServiceProvider (customizing filename of log by php_sapi_name()), should I move the alias to the LogServiceProvider boot method or just keep in the AppServiceProvider?

@garygreen
Copy link
Contributor Author

garygreen commented Feb 11, 2017

@jeffersonsetiawan your problem is with the bugsnag-laravel package and has nothing to do with this PR - you should keep the discussion in the bugsnag/bugsnag-laravel/issues/217 issue. As for whether you should move code into your LogServiceProvider, up to you - if you've created one then it would make sense to keep all log-related bindings and stuff in the LogServiceProvider.

@bedeMarkRadford
Copy link

bedeMarkRadford commented May 4, 2017

@garygreen Does this mean we can no longer use our own Logging Provider?
This is a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants