feat: add more context for Request/Connect/DNSTimeoutErrors #109

DonutEspresso · 2017-02-13T05:45:09Z

Addresses #104.

Here are example errors of both classes:

{ ConnectTimeoutError: GET request to http://169.254.1.10/foo?a=1 failed to obtain connected socket within 100ms
    at buildConnectTimeoutError (/Users/aliu/Sandbox/restify-clients/lib/HttpClient.js:153:12)
    at Timeout.connectTimeout [as _onTimeout] (/Users/aliu/Sandbox/restify-clients/lib/HttpClient.js:231:23)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
  name: 'ConnectTimeoutError',
  jse_shortmsg: 'GET request to http://169.254.1.10/foo?a=1 failed to obtain connected socket within 100ms',
  jse_info: { fullUrl: 'http://169.254.1.10/foo?a=1', connectTimeout: 100 },
  message: 'GET request to http://169.254.1.10/foo?a=1 failed to obtain connected socket within 100ms' }

{ RequestTimeoutError: GET request to http://127.0.0.1:53927/str/request_timeout failed to complete within 150ms
    at buildRequestTimeoutError (/Users/aliu/Sandbox/restify-clients/lib/HttpClient.js:124:12)
    at Timeout.requestTimeout [as _onTimeout] (/Users/aliu/Sandbox/restify-clients/lib/HttpClient.js:383:31)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
  name: 'RequestTimeoutError',
  jse_shortmsg: 'GET request to http://127.0.0.1:53927/str/request_timeout failed to complete within 150ms',
  jse_info:
   { fullUrl: 'http://127.0.0.1:53927/str/request_timeout',
     requestTimeout: 150 },
  message: 'GET request to http://127.0.0.1:53927/str/request_timeout failed to complete within 150ms' }

I'm looking for feedback on two items:

Do we want to attach more fields onto the verror infoobject? The full URL can be parsed by url.parse() to get any of the derived fields (hostname, path, protocol, port, etc.) but I'm happy to directly attach the results of url.parse() to the info object as well.
The initial feedback in connection-related errors could provide more information #104 sounded like we might want to have an underlying error which could be used as a cause for ConnectTimeout or RequestTimeout - but both of these seem pretty low level as is, so I decided to just attach the relevant metadata directly into info object. Would be happy to hear alternative approaches here.

DonutEspresso · 2017-02-13T05:46:16Z

package.json

@@ -48,7 +48,7 @@
    "coveralls": "^2.11.4",
    "eslint": "^3.1.0",
    "istanbul": "^0.4.0",
-    "jscs": "^3.0.0",
+    "jscs": "^2.11.0",


jscs@3.x was hanging for some reason on OSX, and I'm not inclined to spend time trying to figure out why, since it is deprecated already.

coveralls · 2017-02-13T05:50:43Z

Coverage increased (+0.2%) to 84.605% when pulling bbfd640 on verror-info into 85374f8 on master.

coveralls · 2017-02-13T05:50:43Z

Coverage increased (+0.6%) to 86.759% when pulling cfdc819 on verror-info into e0f36ef on master.

DonutEspresso · 2017-02-13T05:51:03Z

@davepacheco took a stab using the initial feedback in your issue. There are some open questions though, would appreciate your thoughts.

DonutEspresso · 2017-02-13T05:53:08Z

lib/HttpClient.js

+// as TooManyRedirects. This has been rectified as TooManyRequests.
+// in the case of restify-clients, we will retain this error type given that
+// users can specify maximum number of redirects to follow.
+if (!errors.TooManyRedirectsError) {


This hasn't changed, I just moved it to the top of the file after the global requires and before function declarations. Also added a comment.

davepacheco · 2017-02-13T19:34:40Z

This looks great! Thanks for providing all the notes about the change. The one extra thing it would be nice to have as an informational property is the request method, since you can't get that from the fullUrl.

Also: if you make a request to a DNS name that was resolved to a specific IP address, is this going to show up in fullUrl using the DNS name or the IP address? We usually find that both are pretty valuable, but the IP address is probably more valuable if we have to choose (because we can figure out what service it was part of if we have the IP, but if we have the DNS name, we can never figure out which instance it was).

Related to #104: there's still the problem that this may not be a connection timeout at all, but rather a DNS timeout. I'm not sure if the name can be changed in a compatible way, but at least the message reflects what really happened here.

Thanks again for doing this. It's going to be a huge help for us.

davepacheco · 2017-02-15T18:16:11Z

Another thing that would be nice in the VError info is the socket's source IP and port so that we can correlate these issues with logs on the server side (even in the case of a connect timeout, because it's possible we handled this connection, only after the client gave up on it).

DonutEspresso · 2018-02-18T04:35:49Z

Sorry for dropping the ball on this! Circled back around and made all the proposed changes, including #111. There are now 3 errors depending on when things go awry: DNSTimeoutError, ConnectTimeoutError, and RequestTimeoutError.

All errors return the following fields on the info object: address, fullUrl, method, port, as well as the timeout value that triggered the error.

DonutEspresso · 2018-02-19T08:34:32Z

I forgot to add - tests won't pass here until restify/errors#86 lands, as we need to standardize on the use of VError (and .info) across all created error types before this PR can work as intended.

hekike · 2018-02-19T21:14:33Z

lib/HttpClient.js

-// jscs:disable maximumLineLength
-var VERSION = JSON.parse(fs.readFileSync(path.join(__dirname, './../package.json'), 'utf8')).version;
-// jscs:enable maximumLineLength
+var VERSION = JSON.parse(fs.readFileSync(


you can require .json files directly:

var VERSION = require(path.join(__dirname, './../package.json')).version;

hekike · 2018-02-19T21:44:50Z

lib/helpers/errors.js

+ * @function createConnectTimeoutErr
+ * @param {Object} opts options object for a request
+ * @param {Object} req the request object
+ * @returns {Object} ConnectionTimeoutError


How about? @returns {verror.VError} ConnectionTimeoutError

hekike · 2018-02-19T21:45:33Z

lib/helpers/errors.js

+ * @function createRequestTimeoutErr
+ * @param {Object} opts options object for a request
+ * @param {Object} req the request object
+ * @returns {Object} RequestTimeoutError


How about? @returns {verror.VError} ConnectionTimeoutError

hekike · 2018-02-19T21:46:02Z

lib/helpers/errors.js

+
+
+
+function createTooManyRedirectsErr(opts, req) {


Missing JSDoc

hekike · 2018-02-19T21:46:35Z

lib/helpers/errors.js

+ * logging/visibility.
+ * @private
+ * @function fullRequestUrl
+ * @param {Object} opts request options


missing JSDoc: options breakdown

hekike

Nice PR, some smaller JSDoc issues

DonutEspresso · 2018-02-20T03:58:30Z

Thanks @hekike, all feedback addressed! Also updated package.json to point restify-errors at the commit needed so that tests will pass. Will point that to latest once a new version is published.

DonutEspresso · 2018-02-20T04:01:34Z

test/errors.js

@@ -21,7 +21,6 @@ var REST_CODES = [
    { name: 'InvalidVersionError', code: 400 },
    { name: 'MissingParameterError', code: 409 },
    { name: 'NotAuthorizedError', code: 403 },
-    { name: 'PreconditionFailedError', code: 412 },


These tests were added to ensure that changes in restify-error didn't break this repo. Since restify-errors is a breaking change, these tests had to be updated (arguably these tests should not belong in this repo, but don't recall the history here).

retrohacker

Looks good to me!

DonutEspresso · 2018-02-22T00:49:30Z

Bunch of work to rebase master. Once tests pass, this is good to go.

DonutEspresso commented Feb 13, 2017

View reviewed changes

DonutEspresso force-pushed the verror-info branch 2 times, most recently from bf0eabe to bbfd640 Compare February 13, 2017 05:48

DonutEspresso requested review from micahr and yunong February 13, 2017 05:48

DonutEspresso commented Feb 13, 2017

View reviewed changes

davepacheco mentioned this pull request Feb 15, 2017

request errors could provide socket details #111

Closed

DonutEspresso force-pushed the verror-info branch from bbfd640 to f70d184 Compare February 17, 2018 09:44

DonutEspresso requested review from retrohacker, hekike and rajatkumar and removed request for micahr and yunong February 18, 2018 04:36

DonutEspresso changed the title ~~add more context for request and connect timeout errors~~ new: add more context for Request/Connect/DNSTimeoutErrors Feb 18, 2018

hekike reviewed Feb 19, 2018

View reviewed changes

lib/helpers/errors.js Outdated

function createTooManyRedirectsErr(opts, req) {

Copy link

Member

hekike Feb 19, 2018

Choose a reason for hiding this comment

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

Missing JSDoc

hekike reviewed Feb 19, 2018

View reviewed changes

hekike requested changes Feb 19, 2018

View reviewed changes

DonutEspresso commented Feb 20, 2018

View reviewed changes

retrohacker approved these changes Feb 20, 2018

View reviewed changes

hekike approved these changes Feb 20, 2018

View reviewed changes

DonutEspresso force-pushed the verror-info branch from 0d1fe86 to a26bc80 Compare February 21, 2018 22:57

DonutEspresso changed the title ~~new: add more context for Request/Connect/DNSTimeoutErrors~~ feat: add more context for Request/Connect/DNSTimeoutErrors Feb 21, 2018

DonutEspresso force-pushed the verror-info branch from a26bc80 to cfdc819 Compare February 21, 2018 23:01

DonutEspresso added 8 commits February 21, 2018 16:39

add more context for request and connect timeout errors

0d1035c

fix: PR comments

f10ad24

fix: add error info for standard http errors as well

1b0f591

fix: pr comments

957071c

fix: pr comments

0135a32

fix: pr comments

f32a04e

fix: use npm published version

d0c0e96

fix: fixed issues with rebase

f66a0e7

DonutEspresso force-pushed the verror-info branch from cfdc819 to f66a0e7 Compare February 22, 2018 00:48

DonutEspresso merged commit 402d72b into master Feb 22, 2018

DonutEspresso deleted the verror-info branch February 22, 2018 00:55

DonutEspresso mentioned this pull request Feb 22, 2018

connection-related errors could provide more information #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add more context for Request/Connect/DNSTimeoutErrors #109

feat: add more context for Request/Connect/DNSTimeoutErrors #109

DonutEspresso commented Feb 13, 2017

DonutEspresso Feb 13, 2017

coveralls commented Feb 13, 2017

coveralls commented Feb 13, 2017 •

edited

DonutEspresso commented Feb 13, 2017

DonutEspresso Feb 13, 2017

davepacheco commented Feb 13, 2017

davepacheco commented Feb 15, 2017

DonutEspresso commented Feb 18, 2018 •

edited

DonutEspresso commented Feb 19, 2018

hekike Feb 19, 2018 •

edited

hekike Feb 19, 2018

hekike Feb 19, 2018

hekike Feb 19, 2018

hekike Feb 19, 2018

hekike left a comment

DonutEspresso commented Feb 20, 2018

DonutEspresso Feb 20, 2018

retrohacker left a comment

DonutEspresso commented Feb 22, 2018

feat: add more context for Request/Connect/DNSTimeoutErrors #109

feat: add more context for Request/Connect/DNSTimeoutErrors #109

Conversation

DonutEspresso commented Feb 13, 2017

DonutEspresso Feb 13, 2017

Choose a reason for hiding this comment

coveralls commented Feb 13, 2017

coveralls commented Feb 13, 2017 • edited

DonutEspresso commented Feb 13, 2017

DonutEspresso Feb 13, 2017

Choose a reason for hiding this comment

davepacheco commented Feb 13, 2017

davepacheco commented Feb 15, 2017

DonutEspresso commented Feb 18, 2018 • edited

DonutEspresso commented Feb 19, 2018

hekike Feb 19, 2018 • edited

Choose a reason for hiding this comment

hekike Feb 19, 2018

Choose a reason for hiding this comment

hekike Feb 19, 2018

Choose a reason for hiding this comment

hekike Feb 19, 2018

Choose a reason for hiding this comment

hekike Feb 19, 2018

Choose a reason for hiding this comment

hekike left a comment

Choose a reason for hiding this comment

DonutEspresso commented Feb 20, 2018

DonutEspresso Feb 20, 2018

Choose a reason for hiding this comment

retrohacker left a comment

Choose a reason for hiding this comment

DonutEspresso commented Feb 22, 2018

coveralls commented Feb 13, 2017 •

edited

DonutEspresso commented Feb 18, 2018 •

edited

hekike Feb 19, 2018 •

edited