Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

Improve logging and error handling #46

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Improve logging and error handling #46

merged 4 commits into from
Jul 7, 2021

Conversation

joao-paulo-parity
Copy link
Contributor

Note: "Draft" while I wait for some ongoing benchmarks to finish before redeploying the bot

Problem

The bot was spotted failing silently recently e.g. in

paritytech/substrate#9018

where it would start the command but never give an actual response of what happened after.

Looking at the logs I see some unknown failures:

		INFO (http): POST / 500 - 1541ms
    err: {
      "type": "Error",
      "message": "failed with status code 500",
      "stack":
          Error: failed with status code 500
              at ServerResponse.onResFinished (/home/benchbot/bench-bot/node_modules/pino-http/logger.js:73:38)
              at ServerResponse.emit (events.js:327:22)
              at ServerResponse.EventEmitter.emit (domain.js:483:12)
              at onFinish (_http_outgoing.js:723:10)
              at onCorkedFinish (_stream_writable.js:673:5)
              at afterWrite (_stream_writable.js:490:5)
              at afterWriteTick (_stream_writable.js:477:10)
              at processTicksAndRejections (internal/process/task_queues.js:83:21)
    }
ERROR (server): Internal Server Error
    Error: Internal Server Error
        at Request.callback (/home/benchbot/bench-bot/node_modules/superagent/lib/node/index.js:883:15)
        at IncomingMessage.<anonymous> (/home/benchbot/bench-bot/node_modules/superagent/lib/node/index.js:1126:20)
        at IncomingMessage.emit (events.js:327:22)
        at IncomingMessage.EventEmitter.emit (domain.js:483:12)
        at endReadableNT (_stream_readable.js:1220:12)
        at processTicksAndRejections (internal/process/task_queues.js:84:21)

Also sometimes shelljs fails

ERROR (ShellJSInternalError): ENOENT: no such file or directory, open '/tmp/shelljs_afe5946f1b74fc61c3cd'
    ShellJSInternalError: ENOENT: no such file or directory, open '/tmp/shelljs_afe5946f1b74fc61c3cd'
        at Object.openSync (fs.js:476:3)
        at Object.readFileSync (fs.js:377:35)
        at execSync (/home/user/w/bench-bot/node_modules/shelljs/src/exec.js:89:17)
        at Object._exec (/home/user/w/bench-bot/node_modules/shelljs/src/exec.js:205:12)
        at Object.exec (/home/user/w/bench-bot/node_modules/shelljs/src/common.js:335:23)
        at BenchContext.self.runTask (/home/user/w/bench-bot/bench.js:24:48)
        at prepareBranch (/home/user/w/bench-bot/bench.js:87:16)
        at async /home/user/w/bench-bot/bench.js:359:23

And also a bug introduced by #42

ERROR (event): report.substring is not a function
  TypeError: report.substring is not a function
      at /home/benchbot/bench-bot/index.js:97:21
      at async Promise.all (index 0)

Solution

In this PR we aim to improve error reporting and logging throughout the code while also trying to make it more concise.

Regarding the above library errors, we'll try to counteract by upgrading all third-party dependencies and favoring native Node.js libraries over them where applicable.

@joao-paulo-parity joao-paulo-parity marked this pull request as ready for review July 7, 2021 12:36
@joao-paulo-parity
Copy link
Contributor Author

We have evidence of it working on Substrate: paritytech/substrate#9286 (comment)

I've also demoed the branch benchmarks in joao-paulo-parity/substrate#1 (comment)

This much is reasonable at the moment for the lack of a proper test suite or static code analysis. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant