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

WIP: Introduce experimental JSON logging #10822

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dominikschubert
Copy link
Member

@dominikschubert dominikschubert commented May 15, 2024

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

  • Logs can now also be streamed to a file in JSON format. This is done in a quite naive way right now, we simply take the current log records, and translate it into a JSON dict which then ends up being streamed as newline-delimited JSON where each line corresponds to one log entry.
  • The current CLI output is generally not affected by this (except by the addition of the raw request logger)

What's left to do:

  • Clean up the code from the initial spike
  • Make this opt-in
  • Make log sink configurable
  • Map keys extracted from the LogRecord to more suitable names
  • Investigate the DEBUG-related code we had to comment out for the aws request/response logging

There will also be some follow ups such as:

  • Add an internal endpoint to stream logs from (e.g. via websocket)
  • Introduce a proper structured logging strategy and rework log levels
  • ... lots more to come in the next few months

@dominikschubert dominikschubert self-assigned this May 15, 2024
@dominikschubert dominikschubert added the semver: patch Non-breaking changes which can be included in patch releases label May 15, 2024
@@ -765,6 +765,7 @@ def create_function(
f"Value {request.get('Runtime')} at 'runtime' failed to satisfy constraint: Member must satisfy enum value set: {VALID_RUNTIMES} or be a valid ARN",
Type="User",
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change

@@ -45,6 +46,7 @@ def __init__(self, service_manager: ServiceManager = None) -> None:
handlers.add_region_from_header,
handlers.add_account_id,
handlers.parse_service_request,
# TODO: add logger that initializes a request "trace"
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
# TODO: add logger that initializes a request "trace"

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 38m 49s ⏱️ + 2m 16s
2 941 tests ±0  2 631 ✅  - 9  303 💤 +2  7 ❌ +7 
2 943 runs  ±0  2 631 ✅  - 9  305 💤 +2  7 ❌ +7 

For more details on these failures, see this check.

Results for commit f19f45c. ± Comparison against base commit 3c4c463.

This pull request skips 2 tests.
tests.aws.services.s3.test_s3.TestS3PresignedPost ‑ test_post_object_with_files
tests.aws.services.s3.test_s3.TestS3PresignedPost ‑ test_s3_presigned_post_success_action_status_201_response

@@ -95,6 +95,10 @@ def _log(self, context: RequestContext, response: Response):
response.status_code,
context.service_exception.code,
extra={
"request_id": context.request_id,
Copy link
Member

Choose a reason for hiding this comment

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

request_id propagation is gonna be crucial for proper grouping and is going to require quite some manual work within service-specific loggers. Other fields can then be inferred based on the request_id because some other fields won't be available at different stages (e.g., service, region, and operation won't be available before parsing an HTTP request into an AWS request; response-related fields such as status code obviously won't be available before handling the request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants