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

Add environment variables to gunicorn access log format #1291

Merged
merged 2 commits into from Jun 4, 2016

Conversation

Projects
None yet
5 participants
@vivianho
Contributor

vivianho commented Jun 2, 2016

Add environment variables to gunicorn access log format using {Variable}e.

Updated the docs to include the new identifier.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jun 3, 2016

Owner

thanks for the patch! But settings.rst shouldn't be edited. Instead you should update the config.py file in the AccessLog class. Can you resend your PR with that change?

Owner

benoitc commented Jun 3, 2016

thanks for the patch! But settings.rst shouldn't be edited. Instead you should update the config.py file in the AccessLog class. Can you resend your PR with that change?

@vivianho

This comment has been minimized.

Show comment
Hide comment
@vivianho

vivianho Jun 3, 2016

Contributor

@benoitc updated docs.

Contributor

vivianho commented Jun 3, 2016

@benoitc updated docs.

@benoitc benoitc merged commit ac708f5 into benoitc:master Jun 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jun 4, 2016

Owner

thanks!

Owner

benoitc commented Jun 4, 2016

thanks!

@tuukkamustonen

This comment has been minimized.

Show comment
Hide comment
@tuukkamustonen

tuukkamustonen Mar 5, 2017

Just saw this released into 19.7.0.

Did you consider the performance impact? Iterating the environment variable map for each log line feels unnecessary as you rarely change the environment variables in-process.

Why not populate it at the startup and re-use later on? I guess the performance impact just is negligible?

tuukkamustonen commented Mar 5, 2017

Just saw this released into 19.7.0.

Did you consider the performance impact? Iterating the environment variable map for each log line feels unnecessary as you rarely change the environment variables in-process.

Why not populate it at the startup and re-use later on? I guess the performance impact just is negligible?

@RonRothman

This comment has been minimized.

Show comment
Hide comment
@RonRothman

RonRothman Mar 5, 2017

RonRothman commented Mar 5, 2017

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 7, 2017

Collaborator

+1 for fixing this in a bugfix release.

Collaborator

berkerpeksag commented Mar 7, 2017

+1 for fixing this in a bugfix release.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Mar 8, 2017

Owner

actually do we know if it's really an issue? Im not sure having the env variable prepopulated vs using the dict will give much (except maybe by writing it to a string first...

All this access logging is fragile anyway and it's why it's not enabled by default (if you use gunicorn behind a proxy you don't need it) . IMO, while we are on it a better fix would be putting the logging in its own thread/process instead of doing it synchronously like we're doing now imo. I have the feeling that any other fix would just distract us from the real one. Thoughts?

Owner

benoitc commented Mar 8, 2017

actually do we know if it's really an issue? Im not sure having the env variable prepopulated vs using the dict will give much (except maybe by writing it to a string first...

All this access logging is fragile anyway and it's why it's not enabled by default (if you use gunicorn behind a proxy you don't need it) . IMO, while we are on it a better fix would be putting the logging in its own thread/process instead of doing it synchronously like we're doing now imo. I have the feeling that any other fix would just distract us from the real one. Thoughts?

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Merge pull request #1291 from lyft/add-environ-variables-to-logger
Add environment variables to gunicorn access log format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment