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

Implement gunicorn access log format #947

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/deployment.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ Options:
--log-level [critical|error|warning|info|debug|trace]
Log level. [default: info]
--access-log / --no-access-log Enable/Disable access log.
--access-log-format TEXT Log format for access logger, check
documentation for details
--use-colors / --no-use-colors Enable/Disable colorized logging.
--proxy-headers / --no-proxy-headers
Enable/Disable X-Forwarded-Proto,
Expand Down
2 changes: 2 additions & 0 deletions docs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ Options:
--log-level [critical|error|warning|info|debug|trace]
Log level. [default: info]
--access-log / --no-access-log Enable/Disable access log.
--access-log-format TEXT Log format for access logger, check
documentation for details
--use-colors / --no-use-colors Enable/Disable colorized logging.
--proxy-headers / --no-proxy-headers
Enable/Disable X-Forwarded-Proto,
Expand Down
22 changes: 21 additions & 1 deletion docs/settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,27 @@ By default Uvicorn uses simple changes detection strategy that compares python f
* `--log-level <str>` - Set the log level. **Options:** *'critical', 'error', 'warning', 'info', 'debug', 'trace'.* **Default:** *'info'*.
* `--no-access-log` - Disable access log only, without changing log level.
* `--use-colors / --no-use-colors` - Enable / disable colorized formatting of the log records, in case this is not set it will be auto-detected. This option is ignored if the `--log-config` CLI option is used.

* `--access-log-format` - Customized message for access log. Will override log formatter for `uvicorn.access` logger. Format string that supports the following placeholders:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding these docs.

* `%(h)s` - remote IP address
* `%(l)s` - prints `-`
* `%(u)s` - user name, currently not supported, prints `-`
* `%(t)s` - request timestamp (in Apache Common Log Format `[10/Oct/2000:13:55:36 -0700]`)
* `%(r)s` - status line (method, path & http version, e.g.)
* `%(m)s` - request method
* `%(U)s` - request path
* `%(q)s` - request query string
* `%(H)s` - request protocol
* `%(s)s` - response status code
* `%(B)s` - response body length (if body was empty, prints `0`)
* `%(b)s` - response body length (if body was empty, prints `-`)
* `%(f)s` - request referer
* `%(a)s` - request user agent
* `%(T)s` - request+response time (in seconds, integer number)
* `%(D)s` - request+response time (in milliseconds, integer number)
* `%(L)s` - request+response time (in seconds, decimal number)
* `%(p)s` - process id (pid) of request handler
* `%({...}i)s` - value of arbitrary request header, e.g. `%({Accept-Encoding}i)s`
* `%({...}o)s` - value of arbitrary response header, e.g. `%({Content-Encoding}o)s`

## Implementation

Expand Down
2 changes: 2 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ coverage==5.5
coverage-conditional-plugin==0.4.0
httpx==1.0.0b0
pytest-asyncio==0.15.1
async_generator==1.10 ; python_version < '3.7'
gunicorn==20.1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#947 (comment)

I don't think we should have any reference to gunicorn on our source code besides the worker.

This comment applies to both the source code and tests. AFAIK, the Gunicorn worker in uvicorn/workers.py is not tested. There have been some attempts to add testing, but they were reverted.

I would suggest removing Gunicorn from requirements.txt, removing test_gunicorn_worker.py, and not depending on Gunicorn directly in tests at this time. Log message format can be tested on uvicorn.access directly. This might simplify the PR and help things move forward.



# Documentation
Expand Down
40 changes: 40 additions & 0 deletions tests/middleware/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,43 @@ async def app(scope, receive, send):
if record.name == "uvicorn.access"
]
assert '"GET / HTTP/1.1" 599' in messages.pop()


@pytest.mark.asyncio
@pytest.mark.parametrize(
"access_log_format,expected_output",
[
# TODO: add more tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the # TODO: add more tests comment. If we want to support all the formatting options independently of Gunicorn, we should test every one, and probably many combinations. See Gunicorn tests/test_logger.py for some ideas.

("access: %(h)s", "access: 127.0.0.1"),
('access: "%({test-request-header}i)s"', 'access: "request-header-val"'),
('access: "%({test-response-header}o)s"', 'access: "response-header-val"'),
],
)
async def test_access_log_format(access_log_format, expected_output, caplog):
async def app(scope, receive, send): # pragma: no cover
assert scope["type"] == "http"
await send(
{
"type": "http.response.start",
"status": 204,
"headers": [(b"test-response-header", b"response-header-val")],
}
)
await send({"type": "http.response.body", "body": b"", "more_body": False})

config = Config(app=app, access_log_format=access_log_format)
with caplog_for_logger(caplog, "uvicorn.access"):
async with run_server(config):
async with httpx.AsyncClient() as client:
response = await client.get(
"http://127.0.0.1:8000",
headers={"test-request-header": "request-header-val"},
)
assert response.status_code == 204

access_log_messages = [
record.message for record in caplog.records if "uvicorn.access" in record.name
]

assert len(access_log_messages) == 1
assert access_log_messages[0] == expected_output
56 changes: 55 additions & 1 deletion tests/protocols/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import socket
import time

import pytest

from uvicorn.protocols.utils import get_client_addr, get_local_addr, get_remote_addr
from uvicorn.protocols.utils import (
RequestResponseTiming,
get_client_addr,
get_local_addr,
get_remote_addr,
)


class MockSocket:
Expand Down Expand Up @@ -91,3 +97,51 @@ def test_get_remote_addr():
)
def test_get_client_addr(scope, expected_client):
assert get_client_addr(scope) == expected_client


def test_request_response_timing_request_duration_seconds():
timing = RequestResponseTiming()
with pytest.raises(ValueError):
timing.request_duration_seconds()

timing.request_started()
with pytest.raises(ValueError):
timing.request_duration_seconds()

# Make sure time.monotonic is updated before calls (caused problems in
# windows tests)
time.sleep(0.02)
timing.request_ended()
assert timing.request_duration_seconds() > 0


def test_request_response_timing_response_duration_seconds():
timing = RequestResponseTiming()
with pytest.raises(ValueError):
timing.response_duration_seconds()

timing.response_started()
with pytest.raises(ValueError):
timing.response_duration_seconds()

# Make sure time.monotonic is updated before calls (caused problems in
# windows tests)
time.sleep(0.02)
timing.response_ended()
assert timing.response_duration_seconds() > 0


def test_request_response_timing_total_duration_seconds():
timing = RequestResponseTiming()
with pytest.raises(ValueError):
timing.total_duration_seconds()

timing.request_started()
with pytest.raises(ValueError):
timing.total_duration_seconds()

# Make sure time.monotonic is updated before calls (caused problems in
# windows tests)
time.sleep(0.02)
timing.response_ended()
assert timing.total_duration_seconds() > 0
87 changes: 87 additions & 0 deletions tests/test_gunicorn_worker.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import random
import re
import signal
import subprocess
import sys
import time

import pytest
import requests


# XXX: copypaste from test_main
# Exclude from coverage, because this code is executed in subprocess.
async def app(scope, receive, send): # pragma: no cover
assert scope["type"] == "http"
await send(
{
"type": "http.response.start",
"status": 204,
"headers": [(b"test-response-header", b"response-header-val")],
}
)
await send({"type": "http.response.body", "body": b"", "more_body": False})


@pytest.mark.xfail(
sys.platform == "win32", reason="gunicorn process doesn't start on windows?"
)
@pytest.mark.parametrize(
"worker_class",
[
"uvicorn.workers.UvicornWorker",
"uvicorn.workers.UvicornH11Worker",
],
)
def test_gunicorn_worker_stdout_access_log_format(worker_class):
random_port = random.randint(1024, 49151)
access_logformat = (
'hellotest %(h)s %(l)s %(u)s %(t)s "%(r)s" %(s)s %(b)s "%(f)s"'
' "%(a)s" %(L)s "%({test-request-header}i)s"'
' "%({test-response-header}o)s"'
)
process = subprocess.Popen(
[
"gunicorn",
"tests.test_gunicorn_worker:app",
"-k=%s" % worker_class,
"--bind=127.0.0.1:%d" % random_port,
"--access-logfile=-",
"--access-logformat=%s" % access_logformat,
],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

attempts = 0
while True:
attempts += 1
try:
resp = requests.get(
"http://127.0.0.1:%d" % random_port,
headers={
"Test-Request-Header": "request-header-val",
"User-Agent": "request-user-agent",
},
)
except requests.exceptions.ConnectionError:
if attempts > 10:
raise
time.sleep(0.05)
else:
break

assert resp.status_code == 204
process.send_signal(signal.SIGTERM)
stdout, stderr = process.communicate()

stdout_lines = stdout.decode().splitlines()
if not len(stdout_lines) == 1:
pytest.fail("Access log line not found, stderr:\n" + stderr.decode())

assert re.match(
r'hellotest 127\.0\.0\.1 - - \[[^]]+\] "GET / HTTP/1\.1" 204 - "-"'
r' "request-user-agent" [0-9.]+ "request-header-val" '
'"response-header-val"',
stdout_lines[0],
)
7 changes: 7 additions & 0 deletions uvicorn/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ def __init__(
log_config: Optional[Union[dict, str]] = LOGGING_CONFIG,
log_level: Optional[Union[str, int]] = None,
access_log: bool = True,
access_log_format: Optional[str] = None,
use_colors: Optional[bool] = None,
interface: InterfaceType = "auto",
debug: bool = False,
Expand Down Expand Up @@ -284,6 +285,8 @@ def __init__(
self.encoded_headers: List[Tuple[bytes, bytes]] = []
self.factory = factory

self.access_log_format = access_log_format

self.loaded = False
self.configure_logging()

Expand Down Expand Up @@ -385,6 +388,10 @@ def configure_logging(self) -> None:
self.log_config["formatters"]["access"][
"use_colors"
] = self.use_colors
if self.access_log_format:
self.log_config["formatters"]["access"][
"fmt"
] = "%(levelprefix)s %(message)s"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be like:

if self.access_log_format:
    self.log_config["formatters"]["access"][
        "fmt"
    ] = self.access_log_format

Copy link
Author

@immerrr immerrr Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite: in Python logging it happens in two stages.

E.g. for logger.info('message %(hello)s %(world)s', {'hello': 1, 'world': 2}):

  • first LogRecord.getMessage will format the message with the args, i.e. expand hello and world and store it as LogRecord.message
class LogRecord:
    def getMessage(self):
        msg = str(self.msg)
        if self.args:
            msg = msg % self.args
        return msg

    def format(self, record):
        record.message = record.getMessage()
        ...
  • then LogRecord fields are formatted using the corresponding Formatter's fmt, that would expand %(message)s that contains message 1 2 pre-formatted in the previous step
class PercentStyle:
    def _format(self, record):
        return self._fmt % record.__dict__

# and

class Formatter:
    def formatMessage(self, record):
        return self._style.format(record)

So right now uvicorn's log formatting and gunicorn access log formatting happen in different stages of processing. They can be made to both work on Formatter level via a LoggingAdapter of sort, but we'll have to sort out some conflicts between the two log formats, --access-log-format coming from gunicorn and uvicorn's own LOGGING_CONFIG.

logging.config.dictConfig(self.log_config)
elif self.log_config.endswith(".json"):
with open(self.log_config) as file:
Expand Down
Loading