Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

logging and import cleanup in process_request.clj #822

Merged
merged 2 commits into from
Jun 26, 2019

Conversation

shamsimam
Copy link
Contributor

Changes proposed in this PR

  • logs backend response status and headers when debug is enabled
  • cleans up unused imports
  • makes logs consistently start with lower-case characters

Why are we making these changes?

Additional logging helps with debugging individual requests. Code cleanup is general hygiene.

@shamsimam shamsimam self-assigned this Jun 25, 2019
cleans up unused imports
makes logs consistently start with lower-case characters
@shamsimam shamsimam force-pushed the debug-response-headers branch from 8ee7415 to d31bd90 Compare June 26, 2019 01:00
@shamsimam shamsimam requested a review from nsinkov June 26, 2019 14:38
@@ -507,6 +512,8 @@
[post-process-async-request-response-fn _ instance-request-properties descriptor instance
{:keys [uri] :as request} reason-map reservation-status-promise confirm-live-connection-with-abort
request-state-chan {:keys [status] :as response}]
(when (utils/request->debug-enabled? request)
(log/info "backend response status:" (:status response)"and headers:" (:headers response)))
Copy link
Contributor

Choose a reason for hiding this comment

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

space missing response)"and headers:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -364,7 +369,7 @@
(histograms/update! (metrics/service-histogram service-id "request-content-length") content-length)
(statsd/inc! metric-group "request_content_length" content-length)))
(catch Exception e
(log/error e "Unable to track content-length on request")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make all these capitalizations a separate PR. Search systematically. Then it will also cover the (log/error "there was a generic error from above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are switching to lower-case. This PR changes the instances in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open a task to replace log/(error|info|warn) .* ?"[A-Z]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created: #825

@nsinkov nsinkov merged commit 7be488c into master Jun 26, 2019
@nsinkov nsinkov deleted the debug-response-headers branch June 26, 2019 16:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants