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

Replace uvicorn access log with our logger #453

Merged
merged 14 commits into from
Sep 29, 2020

Conversation

Hedingber
Copy link
Contributor

@Hedingber Hedingber commented Sep 27, 2020

This PR has 2 targets:

  • Replace uvicorn's access logger with our logger
  • Filter logs of successful /healthz calls (cause soon it will be used periodically by k8s readiness probe)

In FastAPI the way to cause code to do something (logging) on each request and response is writing a middleware.
I wrote a middleware to log before request and after response and therefore we can disable uvicorn's access log

Here are some examples of what will be logged now:

  1. Successful request to /healthz endpoint
  • Before:
INFO:     10.200.0.41:33898 - "GET /api/healthz HTTP/1.0" 200 OK
  • After: nothing will be logged
  1. Failed request to any endpoint (failed with handled exception)
  • Before:
INFO:     10.200.0.26:52330 - "GET /api/projects/bla HTTP/1.0" 405 Method Not Allowed
  • After:
> 2020-09-28 01:37:16,938 [info] Received request: {'method': 'GET', 'client_address': '10.200.0.26:52330', 'http_version': '1.0', 'request_id': 'dce068ff-9502-4c98-beaa-e51249725a05', 'uri': '/api/projects/bla'}
> 2020-09-28 01:37:16,938 [info] Sending response: {'status_code': 405, 'request_id': 'dce068ff-9502-4c98-beaa-e51249725a05', 'uri': '/api/projects/bla', 'method': 'GET'}
  1. Failed request to any endpoint (failed with unhandeled exception)
  • Before:
INFO:     10.200.0.26:58126 - "GET /api/project/bla HTTP/1.0" 500 Internal Server Error
  • After:
> 2020-09-28 01:41:59,653 [info] Received request: {'method': 'GET', 'client_address': '10.200.0.26:58126', 'http_version': '1.0', 'request_id': '025bc236-b511-4f16-8d05-016a58094d1a', 'uri': '/api/project/bla'}
> 2020-09-28 01:41:59,655 [warning] Request handling failed. Sending response: {'status_code': 500, 'request_id': '025bc236-b511-4f16-8d05-016a58094d1a', 'uri': '/api/project/bla', 'method': 'GET'}
  1. Successful request to any endpoint except /healthz
  • Before:
INFO:     10.200.0.41:33898 - "GET /api/projects?full=true HTTP/1.0" 200 OK
  • After:
> 2020-09-28 01:36:04,587 [info] Received request: {'method': 'GET', 'client_address': '10.200.0.26:50880', 'http_version': '1.0', 'request_id': '8e7a5c9b-becd-41c2-8788-e0b221e61dd5', 'uri': '/api/projects?full=true'}
> 2020-09-28 01:36:04,596 [info] Sending response: {'status_code': 200, 'request_id': '8e7a5c9b-becd-41c2-8788-e0b221e61dd5', 'uri': '/api/projects?full=true', 'method': 'GET'}

@Hedingber Hedingber marked this pull request as ready for review September 28, 2020 01:44
Copy link
Contributor

@omesser omesser left a comment

Choose a reason for hiding this comment

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

Since mlrun has its own logger now, access logs should be turned off imo.
Mixed logging with different formats are very confusing and will be problemtic to machine-parse if we ever want to do that

mlrun/api/utils/logger.py Outdated Show resolved Hide resolved
silent_logging_path in path_with_query_string
for silent_logging_path in silent_logging_paths
):
logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.info(
logger.debug(

mlrun/api/main.py Outdated Show resolved Hide resolved
silent_logging_path in path_with_query_string
for silent_logging_path in silent_logging_paths
):
logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.info(
logger.debug(

mlrun/api/main.py Outdated Show resolved Hide resolved
@@ -51,6 +54,63 @@ async def http_status_error_handler(
)


def get_client_addr(scope):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_client_addr(scope):
def get_client_address(scope):

@Hedingber Hedingber merged commit 525ca59 into mlrun:development Sep 29, 2020
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

Successfully merging this pull request may close these issues.

2 participants