feat(logging): improve error message & add link to Node errors page. #106

cloudmu · 2016-08-30T20:03:20Z

This will add error description when logging errors.

The error descriptions are "standard" descriptions supported by Node module errno, see Node Common System Errors and POSIX errno
The error message format is also slightly modified, hopefully to be more beginner friendly. See an error example in the console of a create-react-app test application.

HPM is used by create-react-app for Proxying API Requests. create-react-app expects more beginner friendly error messages. See pull request.

coveralls · 2016-08-30T20:06:06Z

Coverage decreased (-0.3%) to 96.904% when pulling 9614dd0 on cloudmu:master into 28030c5 on chimurai:master.

chimurai · 2016-08-30T21:24:30Z

Hi @cloudmu,

Thanks for the PR.

I do like the improved error messaging.

You already pointed out; Not sure about the extra errno dependency, as it only provides a simple description for the error code.

Below I added two more variants of the error message.

Original error message:
[HPM] PROXY ERROR: ECONNREFUSED. localhost -> http://localhost:3001/api/login

PR error message:
[HPM] Error 'connection refused' (ErrorCode=ECONNREFUSED) occurred while trying to proxy request /api/login from localhost:3000 to http://localhost:3001
Variant without errno dependency
[HPM] An error occurred while trying to proxy request /api/login from localhost:3000 to http://localhost:3001 (ECONNREFUSED)
Variant without errno dependency. With link for more information on the error code.
[HPM] An error occurred while trying to proxy request /api/login from localhost:3000 to http://localhost:3001 (ECONNREFUSED) (https://nodejs.org/api/errors.html#errors_common_system_errors)

My preference would go to variant 3; If the goal is to make the error message less aggressive and more beginner friendly. For the following reasons:

Uninterrupted plain English sentence explaining the error.
Print out original error code for troubleshooting.
Providing link for more info on the error code. (Common System Errors)
No extra errno dependency

What do you think? /cc @gaearon @geowarin

cloudmu · 2016-08-30T21:46:10Z

@chimurai I like variant 3 too. Very clean.

As I suggested in my original create-react-app PR, I was also thinking about a direct link to Nodejs common system error page, but wasn't sure how stable that link would stay. Was waiting on some feedback on preferences from other people, but want to get the ball rolling.

For variant 3, I would probably still point out ECONNREFUSED to be errorcode, and put it on the same line with the link.

So variant 4:
[HPM] An error occurred while trying to proxy request /api/login from localhost:3000 to http://localhost:3001
(ErrorCode=ECONNREFUSED. See https://nodejs.org/api/errors.html#errors_common_system_errors)

What do you think? /cc @gaearon @geowarin

gaearon · 2016-08-30T22:16:36Z

Option 3 looks good to me.

coveralls · 2016-08-30T23:30:08Z

Coverage increased (+0.009%) to 97.188% when pulling 8be5919 on cloudmu:master into 28030c5 on chimurai:master.

coveralls · 2016-08-30T23:30:08Z

Coverage increased (+0.009%) to 97.188% when pulling 8be5919 on cloudmu:master into 28030c5 on chimurai:master.

cloudmu · 2016-08-30T23:36:44Z

Pull request updated for option 3. No extra dependency (errno) just feels right :-)
@chimurai @gaearon Thanks for the quick feedback.

gaearon · 2016-08-31T08:58:33Z

lib/index.js

-        }
+        var errReference = 'https://nodejs.org/api/errors.html#errors_common_system_errors'; // link to Node Common Systems Errors page
+
+        logger.error('[HPM] Error occurred while trying to proxy request %s from %s to %s (%s) \n(%s)', req.url, hostname, target, err.code, errReference);
    }


Do we really need HPM here? If this is a transitive dependency people will be confused what it means.

cloudmu · 2016-08-31T16:25:26Z

Do we really need HPM here? If this is a transitive dependency people will be confused what it means.

HPM is used consistently as the source of logging for other logging levels, so it's a tough call. Some ideas:

option 1: leave HPM out in the error log.
Good for create-react-app use case, but maybe not ideal for other use cases.
option 2: change HPM to http-proxy-middleware.
This makes source of logging less mysterious, and the developers can easily look it up. (It should be done for all log levels for consistency). Btw, http-proxy-middleware does show up as the first search result.
option 3: pass in logSource as argument to the module. This feels an over-kill, and complicates most uses cases.

I like option 2. but @chimurai it's your module, and your opinion counts most :-)

gaearon · 2016-08-31T16:28:32Z

Option 2 sounds good to me.

cloudmu · 2016-08-31T16:48:23Z

@chimurai are you good with option 2 (changing HPM to http-proxy-middleware)?

chimurai · 2016-09-01T13:29:15Z

Option 2 looks good and it's definitely an improvement in HPM.

But I still get the feeling this won't satisfy your original reason for creating this PR.

I think, even with this change, it is still a bit too generic and vague for the create-react-app users.

Another option you might want to explore is to configure a custom error handler.

var proxy = require("http-proxy-middleware");

var onErrorHandler = function (err, req, res) {
    console.log('Something went wrong.');
    console.log('And we are reporting a custom error message.');
};

var options = {target:'http://localhost:3000', onError: onErrorHandler};

https://github.com/chimurai/http-proxy-middleware/blob/master/recipes/proxy-events.md#onerror

With a custom onError handler you can really dive into the specifics, as explained by @gaearon in:
facebook/create-react-app#282 (comment)

This custom onError is not affected by the logLevels. So it's up to you to set it to 'silent' or 'error'...

chimurai · 2016-09-01T13:46:00Z

lib/index.js


-        logger.error('[HPM] PROXY ERROR: %s. %s -> %s', err.code, hostname, targetUri);
+        logger.error('[HPM] Error occurred while trying to proxy request %s from %s to %s (%s) \n(%s)', req.url, hostname, target, err.code, errReference);


Is the line-break really needed?
I can imagine when you configure winston in logProvider to log to a file; those line-breaks will add a lot of visual noise as you go through the logs...

@chimurai good point. I was only thinking console output in create-react-app. I looked there and line-break was used to break long lines.

I agree for the logging purpose we should remove the line-break. I can update the pull request. I understand with your onError suggestion, this pull request won't be necessary for create-react-app use case, but the error message does look a little better overall (plus Node error page link).
Also would you like to use [http-proxy-middleware] or keep [HPM]?

If you can remove the line-break and keep [HPM] this PR is good to go!

Done. Thanks!

gaearon · 2016-09-01T14:16:43Z

I like the onError idea. 👍

cloudmu · 2016-09-01T17:56:58Z

I totally missed the onError in the api. Should use that in the create-react-app use case!

coveralls · 2016-09-01T19:53:02Z

Coverage increased (+0.009%) to 97.188% when pulling a465761 on cloudmu:master into 28030c5 on chimurai:master.

chimurai · 2016-09-01T21:26:26Z

Thanks for the PR.

Don't think you'll need a new version asap if you are using the onError.
Let me know in case you do, than I'll push a new release.

cloudmu · 2016-09-01T23:25:28Z

@chimurai Yes, I am good. No release needed for now.

Thanks for all the discussions and tips.

feat(logging): add error description when logging errors.

9614dd0

feat(logging): remove errno dependency & add link to Node errors page.

8be5919

cloudmu changed the title ~~feat(logging): add error description when logging errors.~~ feat(logging): improve error message & add link to Node errors page. Aug 30, 2016

gaearon reviewed Aug 31, 2016
View reviewed changes

chimurai reviewed Sep 1, 2016
View reviewed changes

feat(logging) remove line-break in error log.

a465761

chimurai merged commit 7d1865c into chimurai:master Sep 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logging): improve error message & add link to Node errors page. #106

feat(logging): improve error message & add link to Node errors page. #106

cloudmu commented Aug 30, 2016

coveralls commented Aug 30, 2016 •

edited

Loading

chimurai commented Aug 30, 2016

cloudmu commented Aug 30, 2016

gaearon commented Aug 30, 2016

coveralls commented Aug 30, 2016 •

edited

Loading

coveralls commented Aug 30, 2016

cloudmu commented Aug 30, 2016

gaearon Aug 31, 2016

cloudmu commented Aug 31, 2016

gaearon commented Aug 31, 2016

cloudmu commented Aug 31, 2016

chimurai commented Sep 1, 2016

chimurai Sep 1, 2016

cloudmu Sep 1, 2016

chimurai Sep 1, 2016

cloudmu Sep 1, 2016

gaearon commented Sep 1, 2016

cloudmu commented Sep 1, 2016

coveralls commented Sep 1, 2016 •

edited

Loading

chimurai commented Sep 1, 2016

cloudmu commented Sep 1, 2016


		logger.error('[HPM] PROXY ERROR: %s. %s -> %s', err.code, hostname, targetUri);
		logger.error('[HPM] Error occurred while trying to proxy request %s from %s to %s (%s) \n(%s)', req.url, hostname, target, err.code, errReference);

feat(logging): improve error message & add link to Node errors page. #106

feat(logging): improve error message & add link to Node errors page. #106

Conversation

cloudmu commented Aug 30, 2016

coveralls commented Aug 30, 2016 • edited Loading

chimurai commented Aug 30, 2016

cloudmu commented Aug 30, 2016

gaearon commented Aug 30, 2016

coveralls commented Aug 30, 2016 • edited Loading

coveralls commented Aug 30, 2016

cloudmu commented Aug 30, 2016

gaearon Aug 31, 2016

Choose a reason for hiding this comment

cloudmu commented Aug 31, 2016

gaearon commented Aug 31, 2016

cloudmu commented Aug 31, 2016

chimurai commented Sep 1, 2016

chimurai Sep 1, 2016

Choose a reason for hiding this comment

cloudmu Sep 1, 2016

Choose a reason for hiding this comment

chimurai Sep 1, 2016

Choose a reason for hiding this comment

cloudmu Sep 1, 2016

Choose a reason for hiding this comment

gaearon commented Sep 1, 2016

cloudmu commented Sep 1, 2016

coveralls commented Sep 1, 2016 • edited Loading

chimurai commented Sep 1, 2016

cloudmu commented Sep 1, 2016

coveralls commented Aug 30, 2016 •

edited

Loading

coveralls commented Aug 30, 2016 •

edited

Loading

coveralls commented Sep 1, 2016 •

edited

Loading