Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add url to retrying log #55

Closed
wants to merge 1 commit into from

Conversation

yatishbalaji
Copy link

Log url while retrying.
This will help application to trace which all apis require retry.
Helps in debugging and improving api performance

@@ -166,7 +166,7 @@ module.exports = async function (url, options) {
return resolve(response);
}

console.error(`Retrying in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt - 1} failed (status ${response.status}): ${response.statusText}`);
console.error(`Retrying ${url} in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt - 1} failed (status ${response.status}): ${response.statusText}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually avoid logging the raw url since it could contain sensitive information. Please redact the sensitive information from the url like this: https://github.com/adobe/asset-compute-commons#examples-2

Copy link
Author

Choose a reason for hiding this comment

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

@jdelbick is there any other way, where we can pass a flag and enable logging or debugging urls?

And let user decide if they want to log or nt

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would suggest using an environment variable (defaulting to not logging the url).

Copy link
Member

Choose a reason for hiding this comment

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

+1

@tmathern tmathern self-requested a review October 27, 2021 21:54
Copy link
Member

@tmathern tmathern left a comment

Choose a reason for hiding this comment

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

We do not log URLs on purpose, as they may leak information (eg. in the case of presigned/SAS URLs).

The suggestion here would be to use and environment variable to change the behavior, no-llogging being the default behavior. Logging URLs must be an explicit action.

@alexkli
Copy link
Contributor

alexkli commented Apr 23, 2022

See #77. With a custom logging option this problem should be solved.

@alexkli
Copy link
Contributor

alexkli commented Apr 23, 2022

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:

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

@alexkli
Copy link
Contributor

alexkli commented Apr 23, 2022

Created #83

@alexkli
Copy link
Contributor

alexkli commented Apr 28, 2022

Closing this, we will address this with #83 and #77.

@alexkli alexkli closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants