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

Improve stack traces thrown from client functions #211

Merged
merged 9 commits into from May 29, 2022

Conversation

Half-Shot
Copy link
Contributor

Currently, stack traces are rather unhelpful when thrown from MatrixClient functions:

     Error: M_FORBIDDEN: Application service has not registered this user (@redacted)
      at /redacted/matrix-bot-sdk/lib/http.js:105:19
      at Generator.next (<anonymous>)
      at fulfilled (/redacted/matrix-bot-sdk/lib/http.js:5:58)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

This doesn't really give away where the problem actually came from in the source. This PR does 3 things to resolve this:

  • Throwing an error doHttpRequest directly, rather than inside the handler function for request. This reduces the amount of cruft in the stack trace. This did require a bit of rejiggling, and also fixed a bug introduced where non-JSON non-200 bodies would cause issues.
  • Using ES2020 for Typescript compilation. This project was on 2015, which meant that async funcs were wrapped in generators. I'd imagine this also comes with a performance improvement, but at the very least using native async gives node a chance to report something sensible.
  • Most importantly: The metrics decorator functions were obscuring the error somewhat with the way they handled calling their parent function. I believe the changes I've introduced don't come with any functionality changes, have definitely improved the stack traces and have tripped down the LOC for each decorator.

The new stack traces look like:

DEBUG 15:46:00:404 [MatrixHttpClient] (REQ-1) PUT http://localhost:8008/_matrix/client/r0/rooms/NotARoom/send/m.room.message/1651761960404__inc1
ERROR 15:46:00:420 [MatrixHttpClient (REQ-1)] { errcode: 'M_FORBIDDEN', error: 'Unknown room' }
ERROR 15:46:00:442 [App] BridgeApp encountered an error and has stopped: MatrixError: M_FORBIDDEN: Unknown room
    at doHttpRequest (/reacted/lib/http.js:95:15)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at MatrixClient.descriptor.value (/reacted/lib/metrics/decorators.js:21:26)
    at MatrixClient.descriptor.value (/reacted/lib/metrics/decorators.js:21:26)
    at MatrixClient.descriptor.value (/reacted/lib/metrics/decorators.js:21:26)
    at MatrixClient.descriptor.value (/reacted/lib/metrics/decorators.js:21:26)
    at MatrixClient.descriptor.value (/reacted/lib/metrics/decorators.js:21:26)
    at Bridge.start (/home/will/git/matrix-hookshot/src/Bridge.ts:91:9)
    at start (/home/will/git/matrix-hookshot/src/App/BridgeApp.ts:39:5) {
  body: { errcode: 'M_FORBIDDEN', error: 'Unknown room' },
  statusCode: 403,
  errcode: 'M_FORBIDDEN',
  error: 'Unknown room',
  retryAfterMs: undefined
}

Which while introduce a bit more cruft when several decorators are used in a chain, it also gives you a hint as to where the function call originated from.

@turt2live turt2live self-requested a review May 5, 2022 15:00
Copy link
Contributor

@tadzik tadzik left a comment

Choose a reason for hiding this comment

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

+1 from me, as this is currently a huge PITA when debugging.

src/http.ts Outdated Show resolved Hide resolved
Half-Shot and others added 2 commits May 5, 2022 16:07
Co-authored-by: Tadeusz Sośnierz <tadzik@tadzik.net>
Copy link
Owner

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

overall lgtm

src/metrics/decorators.ts Outdated Show resolved Hide resolved
src/metrics/decorators.ts Outdated Show resolved Hide resolved
src/metrics/decorators.ts Outdated Show resolved Hide resolved
@turt2live turt2live merged commit 61735cf into turt2live:master May 29, 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.

None yet

3 participants