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

[Issue #2521] Have the error response always return an internal request ID #2523

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

chouinar
Copy link
Collaborator

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make sure we don't lose track of this, either using the X-amzn one in place of this or as an additional identifier (when it's present, and falling back to one we generate when it's missing). But we don't have to change that in this PR, if we want to just open a ticket to follow up in near future Sprint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll have to see whether the x-amzn-requestid field is even populated, that might have been something specific to the past project that this originally came from. I just left that one until we have a chance to dig into it more

@chouinar chouinar merged commit 45a677c into main Oct 23, 2024
@chouinar chouinar deleted the chouinar/2521-request-id branch October 23, 2024 15:36
@coilysiren
Copy link
Collaborator

👀 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return a request ID in the error response to aid in debugging
4 participants