Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Add URI to http response output in debug mode #4808

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

NickLaMuro
Copy link
Contributor

Map request response to original URI when printing debug output for http responses for bundler/fetcher/downloader.

Overview

I have recently been debugging some bundler stalling issues and found this addition to the debug output helpful when ruling out if a particular URL was timing out with the CompactIndex fetcher as it does it's requests over 25 threads and returns them asynchronously. Getting a bunch of HTTP 304 Not Modified isn't as helpful if you are trying to determine how a thread is locking up.

Incidentally, this didn't end up helping solving some issues (further PRs will address this), but it seems like a simple debugging addition that could help rule out specific endpoint issues without requiring more advanced debugging setups.

@@ -14,7 +14,7 @@ def fetch(uri, options = {}, counter = 0)
raise HTTPError, "Too many redirects" if counter >= redirect_limit

response = request(uri, options)
Bundler.ui.debug("HTTP #{response.code} #{response.message}")
Bundler.ui.debug("HTTP #{response.code} #{response.message} #{uri}")
Copy link
Member

Choose a reason for hiding this comment

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

maybe HTTP GET #{uri} => #{response.code} #{response.message} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@segiddins Works for me, and I can make that change. The only problem I do see with it is the URIs plus query params can be quite lengthy, so putting the URI first might make it hard to quickly find 500 errors if the response code/message are at the end. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking in terms of the message being long? ¯_(ツ)_/¯ If in your experience it's been readable with the URL at the end as it is here, then lets :shipit:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, heh. Yeah, usually the message was something like "Not Modified", so the full thing was just "HTTP 304 Not Modified" (without the URI of course).

@segiddins
Copy link
Member

./spec/bundler/fetcher/downloader_spec.rb:35 needs to be updated, otherwise 👍
(I thought response.message was response.body at first)

Useful when mapping completed responses to requests when debugging to
determine which requests are still in progress.
@NickLaMuro NickLaMuro force-pushed the add_url_for_debug_http_response branch from 25fcd31 to 4df7986 Compare July 25, 2016 20:49
@NickLaMuro
Copy link
Contributor Author

@segiddins Sorry about that. Like a Typical Developer™, I didn't run tests locally. Fixed now.

To be fair, had I done so, I would have ran into the issues I have fixed in #4810. ¯\_(ツ)_/¯

@segiddins
Copy link
Member

@homu r+ thanks a bunch!

@homu
Copy link
Contributor

homu commented Jul 26, 2016

📌 Commit 4df7986 has been approved by segiddins

homu added a commit that referenced this pull request Jul 26, 2016
…egiddins

Add URI to http response output in debug mode

Map request response to original URI when printing debug output for http responses for `bundler/fetcher/downloader`.

Overview
--------
I have recently been debugging some bundler stalling issues and found this addition to the debug output helpful when ruling out if a particular URL was timing out with the `CompactIndex` fetcher as it does it's requests over 25 threads and returns them asynchronously.  Getting a bunch of `HTTP 304 Not Modified` isn't as helpful if you are trying to determine how a thread is locking up.

Incidentally, this didn't end up helping solving some issues (further PRs will address this), but it seems like a simple debugging addition that could help rule out specific endpoint issues without requiring more [advanced debugging setups](https://gist.github.com/NickLaMuro/e923fc4e55307437a8f0e97ee6429410).
@homu
Copy link
Contributor

homu commented Jul 26, 2016

⌛ Testing commit 4df7986 with merge 32fa2f7...

@homu
Copy link
Contributor

homu commented Jul 26, 2016

☀️ Test successful - status

@homu homu merged commit 4df7986 into rubygems:master Jul 26, 2016
@coilysiren coilysiren modified the milestone: Release Archive Sep 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants