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

Don't redirect error_log to stderr #940

Open
ViliusS opened this issue Oct 26, 2024 · 6 comments
Open

Don't redirect error_log to stderr #940

ViliusS opened this issue Oct 26, 2024 · 6 comments

Comments

@ViliusS
Copy link

ViliusS commented Oct 26, 2024

Describe the bug

According to https://nginx.org/en/docs/ngx_core_module.html#error_log default error_log level for Nginx should be error, but official docker image is built with level notice. This produces all sorts of issues in logging/monitoring systems. Most often, just graceful ordinary restart produces errors in logs (and then alerts).

Image

To reproduce

Steps to reproduce the behavior:

  1. Run the NGINX Docker image.
  2. Check /var/log/nginx/error.log for startup notices (which are not actually errors).

Expected behavior

error_log should be set to error or maybe warn level, definitely not notice.

Your environment

  • nginx:1.25.1-alpine-slim
  • Kubernetes / Docker Desktop
@oxpa
Copy link
Collaborator

oxpa commented Oct 28, 2024

@ViliusS you are free to change the error log level to any value you prefer.

Though as you can spot from your screenshot nginx logs signals and processes states at a notice log level. Over the years this information has proven itself to be invaluable when it comes to solving issues with nginx. And so we decided to lower logs severity from error to notice in nginx packages.
It is indeed differ from the default in sources though. But I would recommend going with the notice log level whenever possible.

@ViliusS
Copy link
Author

ViliusS commented Oct 28, 2024

OK, thinking about this further, it is a bug in a way error log is redirected to stderr then. If error_log in nginx is not really for errors but for all sorts of notifications and signals one cannot just forward that to stderr. It's due to this redirection logging/monitoring systems then gives false positive alerts.
Either error_log needs to be redirected to stdout (and then parsing of severity levels is left for logging system), OR error_log needs to log only errors. One way or another, current configuration is incorrect.

@ViliusS ViliusS changed the title Incorrect default level for error_log Don't redirect error_log to stderr Oct 28, 2024
@thresheek
Copy link
Collaborator

I'm not entirely following the logic here - why do you think it should not be forwarded to a separate fd at all?

@ViliusS
Copy link
Author

ViliusS commented Oct 28, 2024

I don't think it should not be forwarded at all. I'm saying it should not be forwarded specifically to stderr. stderr is meant to contain errors, and assuming that, container orchestrators on cloud environments (mainly GKE, AKS) forward strerr to monitoring subsystem with severity = error (you can see this in my screenshot). In result, every restart of nginx container under GKE produces 15 error alerts.

@thresheek
Copy link
Collaborator

I mean, it's best to have more information on what's going on than less information. This is why we have it set to something that is verbose but still not spammy.

We definitely won't be changing forwarding error logs to something else other than stderr by default, since it's the expected place for log collections since the inception of this image and systems already depend on this behaviour.

I think in your case it's just a matter of setting up proper filtering for what you'd consider an alert-worthy event.

@ViliusS
Copy link
Author

ViliusS commented Oct 28, 2024

I don't understand your arguments. You say that it is set to "something not spammy", but I'm telling you how the image behaves by default under most containerized conditions and it is the opposite.

The workaround solution is not really easy. One would need to write nginx error_log to JSON converter with severity included for every log message. Only then those messages would be logged at correct severity level on SaaS environments. Filtering alerts by simple string (e.g. "gracefully shuting down" or "exiting") would be insane.

Another way would be to completely rebuild docker image. Which can be done, but I filled this issue wanting to fix the issue at the source, not make another workaround.

I also don't buy the argument that something which was designed wrong in the first place cannot be changed now. At least remove symlink redirect at docker image level and leave the control to nginx configuration. error_log already has special parameter to log directly to stderr. This would keep backward compatibility and would allow nginx configuration override without rebuilding the image.

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

No branches or pull requests

3 participants