WIP: Introduce experimental JSON logging #16
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces experimental JSON logging to LocalStack, focusing on structured log output for improved analysis and debugging.
- Added JsonFormatter class in
localstack/logging/setup.pyfor JSON-formatted logs - Implemented new RequestLogger in
localstack/aws/handlers/logging.pyfor raw HTTP request logging - Modified
setup_loggingfunction to include JSON logging configuration - Added
log_requesthandler to the request chain inlocalstack/aws/app.py - Enhanced log messages with additional fields like request_id, service, and operation
5 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings
| @@ -45,3 +45,4 @@ | |||
| set_close_connection_header = legacy.set_close_connection_header | |||
| push_request_context = legacy.push_request_context | |||
| pop_request_context = legacy.pop_request_context | |||
| log_request = logging.RequestLogger() | |||
There was a problem hiding this comment.
style: Consider adding a comment explaining the purpose of this new logger
| def _prepare_logger(self, logger: logging.Logger, formatter: Type): | ||
| if logger.isEnabledFor(logging.DEBUG): | ||
| logger.propagate = False | ||
| handler = create_default_handler(logger.level) | ||
| handler.setFormatter(formatter()) | ||
| logger.addHandler(handler) | ||
| # TODO: uncommenting this will block .http and .aws logs from being propagated | ||
| # if logger.isEnabledFor(logging.DEBUG): | ||
| # logger.propagate = False | ||
| # handler = create_default_handler(logger.level) | ||
| # handler.setFormatter(formatter()) | ||
| # logger.addHandler(handler) | ||
| return logger |
There was a problem hiding this comment.
style: Commented-out code block prevents propagation of .http and .aws logs. Consider removing or implementing a configuration option to control this behavior.
| if context.request.path == "/health" or context.request.path == "/_localstack/health": | ||
| # special case so the health check doesn't spam the logs | ||
| return |
There was a problem hiding this comment.
style: Duplicate health check logic. Consider extracting this into a separate method to avoid repetition.
| @@ -87,6 +90,32 @@ def create_default_handler(log_level: int): | |||
| return log_handler | |||
|
|
|||
|
|
|||
| class JsonFormatter(logging.Formatter): | |||
There was a problem hiding this comment.
style: JsonFormatter implementation is naive and may not handle all data types correctly. Consider using a more robust JSON serialization method.
| # import re | ||
| # reg = re.compile(r"./localstack/services/([^/])") | ||
| # if "localstack/services/" in record.pathname: | ||
| # service = record.pathname.split("") | ||
| # data["service"] = |
There was a problem hiding this comment.
style: Commented-out code for service extraction. Either implement or remove this block.
| # TODO: make configurable/opt-in | ||
| logging.basicConfig(level=logging.DEBUG) | ||
| file_handler = logging.FileHandler("/tmp/localstack.log", mode="w", encoding="utf-8") |
There was a problem hiding this comment.
style: JSON logging is not configurable and uses a hardcoded file path. Make this configurable as mentioned in the TODO.
|
|
||
| file_handler.setFormatter(JsonFormatter()) | ||
| logging.root.addHandler(file_handler) | ||
| logging.root.setLevel(logging.DEBUG) # FIXME |
There was a problem hiding this comment.
logic: Setting root logger to DEBUG level may be too verbose. Implement proper log level configuration.
Motivation
We've had a few requests in the past for outputting our logs in a structured format. We've been experimenting in the past but would like to push forward with the idea now. This will be in an experimental stage for some time now and will mostly be used internally before going into public preview and ideally GA release with 4.0 🤞
Changes
TODO
What's left to do:
There will also be some follow ups such as: