Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove log decoration from workers output #725

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

lcobucci
Copy link
Contributor

@lcobucci lcobucci commented Oct 10, 2018

As of PHP 7.3 we can finally use STDOUT and STDERR properly in our containers, this disables the [pool %s] child %d said into %s: \"%s\ format.

More info:

@lcobucci
Copy link
Contributor Author

lcobucci commented Oct 12, 2018

@yosifkit as far as I saw the build failures are not related to the changes I've made, should I do something here?

@yosifkit
Copy link
Member

No, the build is fine. The one problem I see is that these changes will be lost as soon as update.sh is run by jenkins. This will need to be added in the template fpm-Dockerfile-block-2 too and then probably excluded via update.sh in older versions.

Would you like to take that on or would you like me to take over from here?

Also, thanks for the contribution! Glad PHP-FPM now has a setting for this. 😄

@lcobucci lcobucci force-pushed the improve-workers-log branch from ac08b45 to e8f6c96 Compare October 12, 2018 22:57
@lcobucci
Copy link
Contributor Author

@yosifkit I feel a bit dumb for not reading the update.sh script before 😂
BTW it's a very helpful script that one 👍

Maybe we could add a simple CONTRIBUTING.md to the repo with some instructions? I can send something tomorrow...

Would you like to take that on or would you like me to take over from here?

I think everything should be fine now but please let me know if there's anything else to change.

Also, thanks for the contribution! Glad PHP-FPM now has a setting for this. smile

My pleasure, I'm also pretty eager to clean up my php-fpm logs 😁

Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

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

LGTM. cc @tianon

We would welcome a helpful contributing.md, if you have the time. (thanks!)

As of PHP 7.3 we finally use STDOUT and STDERR properly in our
containers, this disables the '[pool %s] child %d said into %s: \"%s\'
format.

More info:

- php/php-src#2458
- https://bugs.php.net/bug.php?id=71880
- docker-library#207
@tianon tianon force-pushed the improve-workers-log branch from e8f6c96 to 87c85e4 Compare October 16, 2018 21:08
@tianon tianon merged commit 32575d7 into docker-library:master Oct 16, 2018
@lcobucci lcobucci deleted the improve-workers-log branch October 17, 2018 06:25
tianon added a commit to infosiftr/stackbrew that referenced this pull request Oct 18, 2018
- `drupal`: 8.5.8, 7.60, 8.6.2
- `ghost`: 2.2.4
- `matomo`: 3.6.1
- `openjdk`: 11.0.1
- `php`: remove FPM log decoration on 7.3+ (docker-library/php#725)
- `postgres`: 11.0 GA, 10.5-2.pgdg90+1
- `redis`: EOL 3.2, 5.0.0 GA
- `ruby`: 2.5.3, 2.3.8, 2.4.5
@bukka
Copy link

bukka commented Dec 2, 2018

@lcobucci @tianon @yosifkit Nice that this got integrated! I'm the author of these changes in FPM and wanted suggest to also bum the log_limit which was kept on 1024 just for BC reasons but it would make much more sense to have it set at least to 8192 to not wrap some longer lines (e.g. back traces). I'm currently quite busy to create a PR in here but if some one has got more time for it, that would be great! Otherwise will try to find a time in the next couple of weeks.

@wollanup
Copy link

@bukka What about 502 errors on nginx because of 2048 log size limit ?

I updated some images from php 7.0 to 7.4, and now with default conf I experience 502 if I want to error_log a $throwable->__toString();

Had to edit /usr/local/etc/php-fpm.d/docker.conf and set something under 2048.

Is this an expected behavior ?

@lunfel
Copy link

lunfel commented Oct 14, 2020

I use Laravel with a docker-compose setup and when I use the errorlog logging channel (Laravel logging configuration), I get 502 on requests that have exceptions. It seems to be aligned with what @wollanup is saying about throwable.

From nginx I kept having upstream sent too big header while reading response header from upstream.

As soon as I changed the logging channel to something else, it started working again (proper 500 error)

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

Successfully merging this pull request may close these issues.

6 participants