consistent URL logging approach

[alexkli]

From #55

Note that node-fetch-retry is inconsistent and includes URLs in errors thrown (e.g. here). It's fair to assume that all errors get logged downstream. So if the policy is to hide URLs by default, then they should not be included in errors either.

Also it logs the FetchErrors (e.g. here) which sometimes contain URL as well:

FetchError failed with code: ECONNRESET; message: request to https://www.adobe.com/content/dam/shared/images/product-icons/svg/substance-3d-designer.svg failed, reason: read ECONNRESET
Retrying in 103 milliseconds, attempt 1 error: FetchError, request to https://www.adobe.com/content/dam/shared/images/product-icons/svg/substance-3d-designer.svg failed, reason: read ECONNRESET

With a custom logging option (#77) in place, there are two options:

1. log urls by default, and let clients with sensitive cases add a custom logger function that skips urls
2. do not log urls by default, and let clients who want to log urls add a custom logger function that does so

But for convenience there could also be a third option:

3. add extra flag logDetails or logUrl that would be maybe off by default if security conscious

[alexkli]

Please vote on hiding urls by default using reactions on this comment.

Just on the basic rule of showing urls in logs by default or not (ignore mention of other options above).

👍 for HIDING urls in all logs and errors thrown by default

• non trivial work: requires wrapping all errors and "remove" the url information while retaining the rest of the error message, plus a flag to turn this off for those that want the urls in e.g. the errors thrown

👎 for SHOWING urls in logs by default

• clients who don't want to log them need to avoid logging complete errors in their catch handlers (which they need for plain fetch() anyway), and would need their custom logging handler allow customizing of retry logging handler #77 which doesn't log urls/does not log anything

cc @tmathern @jdelbick

[trieloff]

I'd like to express a soft preference for the secure-by-default stance of not logging URLs (or at least URL parameters) by default, but given that I'll carry no burden in implementing any of it, feel free to dismiss it. Having sensitive information in the URL is the core of the problem, and it is unlikely that the logging in this library is going to break the camel's back.

[alexkli]

Another approach would be to not log anything by default, and basically follow suit how fetch() libraries/implementations work:

• they don't console.log anything
• they can throw errors including URLs, but the logging of these is up to the client code

If someone wants to see the retries in the logs, they can do so with #77 implemented.

Downside is that by default you wouldn't see what's going on, and for Nui as consumer in particular, we'd loose the logging if we update, and would have to either add the custom logging handler everywhere node-fetch-retry is used, or use another new intermediary library...

We could also include the logging code, but offer a flag to enable (some standard) logging.

Thoughts?

@tmathern @jdelbick

[trieloff]

In the vein of not logging anything by default, it could make sense to introduce some event listeners, so that you can:

const fetch = require('...');

fetch.on('fetch', console.log);
fetch.on('error', console.error);
fetch.on('retry', (r) => {
  if (r.attempt > 100) {
    throw new Error('This is getting ridiculous');
  }
});

I'm thinking the custom retry handler would also help if you want to combine the retry-logic with a queuing logic.

[alexkli]

@trieloff

event listeners

Yes, that's what #77 wants to offer more or less. Interesting to have more event types than just retry.

combine the retry-logic with a queuing logic.

I wonder how that could work - you'd not want node-fetch-retry to actually retry in this case, so you'd have to add the retry to a queue, tell node-fetch-retry to not retry at all. But then the retry state would be lost and things like the maxRetryDuration would be hard to implement?