Skip to content

Split LogRequests middleware into multiple middlewares #3898

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

Closed
wants to merge 4 commits into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Sep 7, 2021

The LogRequests middleware is currently performing multiple tasks:

  • Save req.path() before the NormalizePath middleware potentially mutates the request path
  • Calculate response timing based on the x-request-start HTTP header from Heroku (or the StartInstant extension as a fallback)
  • Log requests to stdout
  • Report errors to Sentry

The Sentry error reporting was originally built into the LogRequests middleware because it also needed access to the original path and the response timing.

This PR proposes to split the LogRequests middleware, so that each middlewares is performing only one of the functions respectively:

  • The "original path" code is moved into the NormalizePath middleware that is responsible for mutating the path in the first place
  • The request timing code is moved into a new RequestTiming middleware, which saves the response time in the request extension map
  • The Sentry error reporting code is moved into a new SentryMiddleware

@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Sep 7, 2021
@Turbo87 Turbo87 requested review from jtgeibel and 0xPoe September 10, 2021 15:20
@bors
Copy link
Contributor

bors commented Sep 11, 2021

☔ The latest upstream changes (presumably #3904) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 force-pushed the split-middleware branch 3 times, most recently from 5b0f4d9 to c2cca5a Compare September 17, 2021 21:46
@Turbo87 Turbo87 force-pushed the split-middleware branch 3 times, most recently from 9a28d52 to 2a10ef7 Compare September 26, 2021 08:34
@Turbo87 Turbo87 force-pushed the split-middleware branch 3 times, most recently from 5e2022e to 513e490 Compare October 5, 2021 19:37
@bors
Copy link
Contributor

bors commented Oct 14, 2021

☔ The latest upstream changes (presumably #3988) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 19, 2021

☔ The latest upstream changes (presumably #4036) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 7, 2021

closing in favor of #4137

@Turbo87 Turbo87 closed this Nov 7, 2021
@Turbo87 Turbo87 deleted the split-middleware branch October 17, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants