-
Notifications
You must be signed in to change notification settings - Fork 647
middleware::log_request: Split request timing code into dedicated middleware #4116
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
Conversation
☔ The latest upstream changes (presumably #4115) made this pull request unmergeable. Please resolve the merge conflicts. |
16cda83
to
fd05665
Compare
fd05665
to
775a75a
Compare
if let Some(response_time) = response_time { | ||
if response_time.as_millis() > SLOW_REQUEST_THRESHOLD_MS { | ||
line.add_marker("SLOW REQUEST")?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could de-duplicate this if let
by merging it into the above if let
or binding response_time
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with merging them is that it changes the field order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, then it should be fine. Thanks for clarifying!
@bors r+ |
📌 Commit 775a75a has been approved by |
☀️ Test successful - checks-actions |
This is generic and not strictly related to the
LogRequest
middleware. TheUpdateMetrics
middleware should probably also use this in the future.