From 45a677cfdcc733202cde7913ad8dffbeb5503e4b Mon Sep 17 00:00:00 2001 From: Michael Chouinard <46358556+chouinar@users.noreply.github.com> Date: Wed, 23 Oct 2024 11:36:25 -0400 Subject: [PATCH] [Issue #2521] Have the error response always return an internal request ID (#2523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #2521 ### Time to review: __3 mins__ ## Changes proposed Return the internal request ID in all error responses ## Context for reviewers A follow-up from a discussion we had yesterday. This can aid in debugging as a user can just give us the ID and we can easily look up the corresponding logs. Note `g` in Flask is a request-local global context (ie. each request has their own separate `g`) which we use for storing a few other logging related things already safely. https://flask.palletsprojects.com/en/3.0.x/appcontext/#storing-data ## Additional information Screenshot 2024-10-18 at 12 15 49 PM Screenshot 2024-10-18 at 12 16 09 PM Screenshot 2024-10-18 at 12 17 05 PM An example of the logs that show the internal ID - note I trimmed out the full error message as it's a very long stack trace. ![Screenshot 2024-10-18 at 12 17 25 PM](https://github.com/user-attachments/assets/e923f84d-e295-4719-9d80-65ac98f13ca8) --------- Co-authored-by: nava-platform-bot --- api/openapi.generated.yml | 4 ++++ api/src/api/response.py | 2 ++ api/src/api/schemas/response_schema.py | 6 ++++++ api/src/logging/flask_logger.py | 9 ++++++--- api/tests/src/api/test_healthcheck.py | 1 + 5 files changed, 19 insertions(+), 3 deletions(-) diff --git a/api/openapi.generated.yml b/api/openapi.generated.yml index 894397730..f70c7bc12 100644 --- a/api/openapi.generated.yml +++ b/api/openapi.generated.yml @@ -587,6 +587,10 @@ components: - object allOf: - $ref: '#/components/schemas/ValidationIssue' + internal_request_id: + type: string + description: An internal tracking ID + example: 550e8400-e29b-41d4-a716-446655440000 OpportunitySorting: type: object properties: diff --git a/api/src/api/response.py b/api/src/api/response.py index f85fc0f7c..52f4ebd22 100644 --- a/api/src/api/response.py +++ b/api/src/api/response.py @@ -3,6 +3,7 @@ from typing import Any, Optional, Tuple import apiflask +import flask from src.api.schemas.extension import MarshmallowErrorContainer from src.pagination.pagination_models import PaginationInfo @@ -92,6 +93,7 @@ def restructure_error_response(error: apiflask.exceptions.HTTPError) -> Tuple[di # we rename detail to data so success and error responses are consistent "data": error.detail, "status_code": error.status_code, + "internal_request_id": getattr(flask.g, "internal_request_id", None), } validation_errors: list[ValidationErrorDetail] = [] diff --git a/api/src/api/schemas/response_schema.py b/api/src/api/schemas/response_schema.py index 928ef19ab..3f267ec3b 100644 --- a/api/src/api/schemas/response_schema.py +++ b/api/src/api/schemas/response_schema.py @@ -52,3 +52,9 @@ class ErrorResponseSchema(Schema): errors = fields.List( fields.Nested(ValidationIssueSchema()), metadata={"example": []}, dump_default=[] ) + internal_request_id = fields.String( + metadata={ + "description": "An internal tracking ID", + "example": "550e8400-e29b-41d4-a716-446655440000", + } + ) diff --git a/api/src/logging/flask_logger.py b/api/src/logging/flask_logger.py index 4a848a48a..92dd005d6 100644 --- a/api/src/logging/flask_logger.py +++ b/api/src/logging/flask_logger.py @@ -154,14 +154,17 @@ def _add_global_context_info_to_log_record(record: logging.LogRecord) -> bool: def _get_request_context_info(request: flask.Request) -> dict: + internal_request_id = str(uuid.uuid4()) + flask.g.internal_request_id = internal_request_id + data = { "request.id": request.headers.get("x-amzn-requestid", ""), "request.method": request.method, "request.path": request.path, "request.url_rule": str(request.url_rule), - # A backup ID in case the x-amzn-requestid isn't passed in - # doesn't help with tracing across systems, but at least links within a request - "request.internal_id": str(uuid.uuid4()), + # This ID is used to group all logs for a given request + # and is returned in the API response for any 4xx/5xx scenarios + "request.internal_id": internal_request_id, } # Add query parameter data in the format request.query. = diff --git a/api/tests/src/api/test_healthcheck.py b/api/tests/src/api/test_healthcheck.py index 70e699b99..56029b0b2 100644 --- a/api/tests/src/api/test_healthcheck.py +++ b/api/tests/src/api/test_healthcheck.py @@ -18,3 +18,4 @@ def err_method(*args): response = client.get("/health") assert response.status_code == 503 assert response.get_json()["message"] == "Service Unavailable" + assert response.get_json()["internal_request_id"] is not None