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

Show more info on RequestError #1549

Closed
wants to merge 1 commit into from
Closed

Show more info on RequestError #1549

wants to merge 1 commit into from

Conversation

erfanium
Copy link

@erfanium erfanium commented Dec 8, 2020

Related #1517

In production, sometimes some requests fail and current RequestError hasn't enough info to troubleshoot.

This is a draft PR, Still has some work to do, But before I finish, I want to know your opinion.
Thanks!

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@szmarczak
Copy link
Collaborator

In production, sometimes some requests fail and current RequestError hasn't enough info to troubleshoot.

What do you mean? It has request and response properties which can be accessed via the way you did this in this PR, so this is moot.

@erfanium
Copy link
Author

erfanium commented Dec 8, 2020

@szmarczak
request and response objects are non-enumerable properties, They will not logged by default, look at this very simple example:

const got = require('got')
const doSomething = () => got('https://google.com/notexist');

(async () => {
   try {
      await doSomething()
   } catch(e) {
      console.error(e)
   }
})()

And logs:

HTTPError: Response code 404 (Not Found)
    at Request.<anonymous> (C:\Users\erfan\Desktop\backend\node_modules\got\dist\source\as-promise\index.js:117:42)
    at processTicksAndRejections (internal/process/task_queues.js:93:5) {
  code: undefined,
  timings: {
    start: 1607441410085,
    socket: 1607441410089,
    lookup: 1607441410274,
    connect: 1607441410463,
    secureConnect: 1607441410699,
    upload: 1607441410699,
    response: 1607441410911,
    end: 1607441410915,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 4,
      dns: 185,
      tcp: 189,
      tls: 236,
      request: 0,
      firstByte: 212,
      download: 4,
      total: 830
    }
  }
}

There's nothing about request method, url, headers, body... in logs.
One possible solution is to check Error type in catch block, if it's instance of RequestError then log it differently:

const got = require('got')
const doSomething = () => got('https://google.com/notexist');

(async () => {
   try {
      await doSomething()
   } catch(e) {
      if (e instanceof got.RequestError) {
         console.error(e, e.request, e.response)
      } else console.error(e)
   }
})()

This works fine, And I have used this method so far, but I should always be careful to check this condition wherever I log. for example I need to modify fastify default errorHandler just for this, I think this simple change can solve many problems .

@szmarczak
Copy link
Collaborator

request and response objects are non-enumerable properties, They will not logged by default, look at this very simple example:

That's right. The solution is to mark them as enumerable and mark only some properties non enumerable inside them. I'm going to fix this in #1521

@szmarczak
Copy link
Collaborator

#1517 is about retryCount. It would be nice to have error.retryCount so people don't have to error.response ? error.response.retryCount : 0 every single time.

@erfanium erfanium closed this Dec 13, 2020
@erfanium erfanium mentioned this pull request Mar 24, 2021
71 tasks
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.

2 participants