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

Honor gunicorn access log format #527

Open
ipmb opened this issue Dec 13, 2019 · 20 comments · May be fixed by #947
Open

Honor gunicorn access log format #527

ipmb opened this issue Dec 13, 2019 · 20 comments · May be fixed by #947

Comments

@ipmb
Copy link
Contributor

ipmb commented Dec 13, 2019

When running under gunicorn, --access-logformat isn't honored.

There was some discussion of this in #389, but I didn't see an open issue for it.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@danieltahara
Copy link

This is a pretty big loss for observability for us, and means we have to implement something out of band (and therefore less efficient). Any word on if it will be worked on?

@loudsquelch
Copy link

Any update on this?

@asgoel
Copy link

asgoel commented Jun 8, 2020

Any update on this one?

@tomchristie
Copy link
Member

Hey folks - "Any update on this" is almost always not helpful. If there was an update on it, it'd be here on the ticket.
If anyone wants to put some work into progressing it that'd be welcome.

@asgoel
Copy link

asgoel commented Jun 9, 2020

@tomchristie I would be willing to take a stab. i've taken a look at the code and it seems like the UvicornWorker is fundamentally written in a different way than the rest of the gunicorn workers that come bundled by default.

We would basically need to thread through the Logger instance from gunicorn and call the access method when the request is complete in httptools_impl/h11_impl. We'd also need to record request time which the app currently does not do.

Does this approach sound somewhat correct to you?

@asgoel
Copy link

asgoel commented Jul 9, 2020

@tomchristie just following up to see if my above comment made sense.

@KlausGlueckert
Copy link

We have the same problem at the moment and are about to write a custom logging engine for this, because we must be able to alter the uvicorn log format when running it from gunicorn. Any update on this if there will be a fix? In addition, ideally it should also be configurable in a logging.conf:

[formatter_AccessFormatter]
format=|| %(levelname)s || %(message)s
datefmt=%Y-%m-%dT%H:%M:%S
class=logging.Formatter
access_log_format = %(h)s %(l)s %(u)s %(t)s %(r)s %(s)s %(b)s %(f)s %(a)s

@immerrr
Copy link

immerrr commented Feb 3, 2021

I have implemented the gunicorn logging in a very naive and straightforward way in my branch.

Would you be interested in a PR?

@euri10
Copy link
Member

euri10 commented Feb 3, 2021

yes @immerrr !
can't promise a speedy speedy review though, pretty bysu

@immerrr
Copy link

immerrr commented Feb 3, 2021

@euri10 sure, np, here you go: #947

@immerrr immerrr linked a pull request Feb 3, 2021 that will close this issue
@chbndrhnns
Copy link

Now that there is a PR, what would be required to add this functionality? We'd like to match our existing aiohttp-created logs with some new logs created from a FastAPI -> uvicorn -> gunicorn stack.

@euri10
Copy link
Member

euri10 commented May 27, 2021

Now that there is a PR, what would be required to add this functionality? We'd like to match our existing aiohttp-created logs with some new logs created from a FastAPI -> uvicorn -> gunicorn stack.

tl;dr : not a priority on my side at all

The longer version is I'm lacking time mostly, plus I dont use gunicorn nor am familiar enough with its internals to have a good idea if the implementation is correct or sensible. On top of that there are 0 tests for the feature which makes me very hesitant since this part of the code (UvicornWorker) is already weakly tested in our codebase.

@immerrr
Copy link

immerrr commented May 27, 2021

@euri10 the low priority thing is understandable. i asked it in the PR, but let me reiterate, is there anything that would help to get the ball rolling here?

i mean, if it's the gunicorn part, maybe we can ask nicely in the gunicorn community to have someone come over and have a look. if it's the tests, then i can write them risking that the test code will be thrown away if the approach is not solid enough, but it's fine, i guess. WDYT?

@euri10
Copy link
Member

euri10 commented May 28, 2021

I answered in the PR @immerrr

immerrr added a commit to immerrr/uvicorn that referenced this issue May 31, 2021
immerrr added a commit to immerrr/uvicorn that referenced this issue Jun 14, 2021
immerrr added a commit to immerrr/uvicorn that referenced this issue Jun 21, 2021
immerrr added a commit to immerrr/uvicorn that referenced this issue Aug 25, 2021
immerrr added a commit to immerrr/uvicorn that referenced this issue Nov 12, 2021
@lambdaq
Copy link

lambdaq commented Dec 22, 2021

Can we merge this PR? This is extremely useful for me.

@KaranTrivedi
Copy link

I just had my issue merged into this thread. Any updates on this?

immerrr added a commit to immerrr/uvicorn that referenced this issue Feb 10, 2022
@bhumkong
Copy link

bhumkong commented Jul 19, 2022

While this is not implemented, asgi-logger might be useful to customize access log format.
It supports showing response time.

@suhcrates-web
Copy link

I got the answer.
see that gunicorn anyway writes log records, and it follows certain default format. And such default format is configured in somwhere. Then, you can modify that config.
then, where is the default config? it is in the /sitepackages/gunicorn/glogging.py where the gunicorn is installed. it is the 'access_fmt' variable in the 'Logger' class.

@pythonwood
Copy link

gunicorn support async base worker offically now. may be is time to rewrite UvicornWorker(AsyncWorker).

AsyncWorker source:

class AsyncWorker(base.Worker):
     ...
    def handle_request(self, listener_name, req, sock, addr):
                ...
                request_time = datetime.now() - request_start
                self.log.access(resp, req, environ, request_time)
     

it is so long to wait. #606
may be i will try it

@waketzheng
Copy link

I am using FastAPI, and this worked for me:
tiangolo/fastapi#1508 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.