-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
Disable access logs for health endpoints by default #448
Comments
Upon closer investigation, it looks like the health endpoints should be excluded already: https://github.com/ory/hydra/blob/f588ec69d4fa03f602d3cbb20abd4188195a7375/cmd/server/handler.go#L204 I'll investigate a bit deeper. Looks like you have to explicitly disable it: https://github.com/ory/hydra/blob/f588ec69d4fa03f602d3cbb20abd4188195a7375/driver/config/serve.go#L71 |
So it looks like this was my fault and I didn't investigate deeply enough before creating this; it's actually documented in https://www.ory.sh/hydra/docs/reference/configuration/ access_log:
## disable_for_health ##
#
# Disable access log for health endpoints.
#
# Default value: false
#
# Set this value using environment variables on
# - Linux/macOS:
# $ export SERVE_ADMIN_ACCESS_LOG_DISABLE_FOR_HEALTH=<value>
# - Windows Command Line (CMD):
# > set SERVE_ADMIN_ACCESS_LOG_DISABLE_FOR_HEALTH=<value>
#
disable_for_health: false With that in mind, I wonder how feasible it would be to disable this by default. Does anybody truly benefit from having access logs enabled for health endpoints? |
Thank you for the report! Indeed, the default is that all logs are available. That way you can investigate easily wether e.g. health checks pass. This can help when services start up and are not responsive. If you have high traffic websites or production deployments, it usually makes sense to disable them. Given that this option is available and documented (although we could probably document it better), do you feel that this issue can be closed? |
For clarification, changing this to default to true might improve the experience for new users who expect this to be the case, but it will be a breaking change for people who rely on this feature. So we won't be changing the default, but it would make sense to better document it. |
I'm closing this as there are currently no plans to change the default |
Preflight checklist
Describe the bug
Access logs from Kubernetes' readiness and liveness probes are polluting the logs, which makes it harder to debug issues.
Reproducing the bug
Relevant log output
section below.Relevant log output
Relevant configuration
No response
Version
v1.10.6-sqlite
On which operating system are you observing this issue?
No response
In which environment are you deploying?
Kubernetes with Helm
Additional Context
I believe that the endpoints used for health checks (
/health/ready
and/health/alive
) should not be logged by the middleware taking care of access logs.There is already a feature in place to enable this, and it is used to ignore the
/ping
endpoint:x/reqlog/middleware.go
Line 93 in f7381ee
Update
See comments below; it turns out there is a way to remove access logs for health endpoints (
serve.public.access_logs.disable_for_health
andserve.admin.access_logs.disable_for_health
), so instead, I'll like to know if there's really any reason to not enabling these by default? Just fixing it for my project is trivial -- it's just a configuration change -- but I feel that disabling this by default might make the experience a bit simpler for devs.The text was updated successfully, but these errors were encountered: