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

Keep default log_limit value #1261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rolandsusans
Copy link

@rolandsusans rolandsusans commented Mar 9, 2022

PHP default configuration says that it should be 1024: https://www.php.net/manual/en/install.fpm.configuration.php

image

I think that we need to make things consistent as close as possible to defaults coming from PHP itself, as these are PHP base images.

Specifying different log_limit should be a project-specific thing, and it should not be specified in the base image.

I see that changes were introduced in #725 (comment).

@rolandsusans
Copy link
Author

rolandsusans commented Mar 9, 2022

@yosifkit Do you think that the php alpine base image should stick to the defaults provided in PHP docs?

@tianon
Copy link
Member

tianon commented Mar 9, 2022

This is definitely more complicated than just resetting this value back to the default -- see #878 (comment), which has some good details about this particular land-mine. 🙈

@rolandsusans
Copy link
Author

@tianon Thanks for pointing to the comment!

There are multiple things being discussed:

  • multiple error messages being logged ( nginx, php-fpm) fastcgi.logging = Off and how to fix them.
  • changing log_limit value was mentioned ( as some partial fix )

I'm reaching out to solve "the default configuration issue" here.
There is a default configuration for log_limit specified log_limit = 8192

docker run php:8.0.16-fpm-alpine cat /usr/local/etc/php-fpm.d/docker.conf

result:

[global]
error_log = /proc/self/fd/2

; https://github.com/docker-library/php/pull/725#issuecomment-443540114
log_limit = 8192

[www]
; if we send this to /proc/self/fd/1, it never appears
access.log = /proc/self/fd/2

clear_env = no

; Ensure worker stdout and stderr are sent to the main error log.
catch_workers_output = yes
decorate_workers_output = no

Seems that specifying such log_limit value is a project-specific thing for the author and such a setting should not be part of the base image, cause it should be as clean as possible and close to PHP default values.

Such a setting makes users of base images override this value when they want to stick to PHP default values.

Does it make sense? How can I contribute more to the "the default configuration issue" topic?

@yosifkit
Copy link
Member

The default was changed for the image because the PHP maintainer that made the "decorate_workers" improvements in FPM said that the only reason they didn't increase the default line length was for backwards compatibility and commented that a larger value would be generally more useful (#725 (comment)).

Since this has been the default in the container images for over three years, setting this to a smaller value could unexpectedly break users' logging setups that rely on the PHP logging to not arbitrarily add extra newlines (e.g. using ELK to detect a backtrace and gather the related lines). So, no I don't think this value should be changed to a smaller value.

@rolandsusans
Copy link
Author

rolandsusans commented Mar 14, 2022

@yosifkit Keeping backwards compatibility makes sense. 👍🏻

Maybe it also makes sense to make this deviation from PHP defaults more visible, by explaining the "default" configuration of docker.conf and other configs?

I'm looking forward to contributing here. Any suggestions on how we could make this more transparent for anyone?

(The issue I'm trying to solve here: "the conf is not well documented & it is hacky to override log_limit")

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.

3 participants