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

Runtime: better errors #728

Merged
merged 14 commits into from
Jul 5, 2024
Merged

Runtime: better errors #728

merged 14 commits into from
Jul 5, 2024

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Jul 2, 2024

We've had ongoing problems getting errors out of the runtime, with too much information being obscured.

There's a few reasons for this - the biggest one being that I've designed error handling around Error objects, but errors aren't actually serializable, with mixed results.

This PR basically serializes an error before a) logging it and b) writing it to state.

This ensures that the error is visible in the logs and on the resulting object.

What is nice as that as well as having more content, the updated tests have, IMO, cleaner error objects.

#726 is tracking a better, more holistic fix in this area. But this gets us out of hot water for now relatively cheaply.

Issues

Closes #720 #612 OpenFn/adaptors#624

Screenshots

401s coming out of odk:

This is important because it shows the common http error object getting serialzied nicely (it also writes to state)

[CLI] ♦ Versions:
         ▸ node.js                 18.12.1
         ▸ cli                     1.4.2
         ▸ @openfn/language-odk    monorepo
[CLI] ✔ Loading adaptors from monorepo at /d/repo/openfn/adaptors
[CLI] ⚠ Skipping auto-install as monorepo is being used
[CLI] ✔ Compiled all expressions in workflow
WARNING: no authentication properties found on congfiguration schema!
    
Make sure to either set an access_token or email & password
[R/T] ✘ Failed step job-1 after 745ms
[R/T] ✘ [AdaptorError: GET to https://sandbox.getodk.cloud/v1/projects/66/forms returned 401: Unauthorized] {
  source: 'runtime',
  type: 'AdaptorError',
  severity: 'fail',
  details: Error: GET to https://sandbox.getodk.cloud/v1/projects/66/forms returned 401: Unauthorized
      at assertOK (/d/repo/openfn/adaptors/packages/common/dist/util.cjs:93:19)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async request (/d/repo/openfn/adaptors/packages/common/dist/util.cjs:158:3)
      at async /d/repo/openfn/adaptors/packages/odk/dist/index.cjs:157:22
      at async file:///d/repo/openfn/kit/packages/runtime/dist/index.js:581:20
      at async file:///d/repo/openfn/kit/packages/runtime/dist/index.js:545:20 {
    statusCode: 401,
    statusMessage: 'Unauthorized',
    url: 'https://sandbox.getodk.cloud/v1/projects/66/forms',
    duration: 595,
    method: 'GET',
    body: {
      message: 'Could not authenticate with the provided credentials.',
      code: 401.2
    },
    headers: {
      server: 'nginx',
      date: 'Tue, 02 Jul 2024 14:37:19 GMT',
      'content-type': 'application/json; charset=utf-8',
      'content-length': '80',
      connection: 'keep-alive',
      etag: 'W/"50-ZRrLMzj8S67I1VD1Vnbql2PHY1Y"',
      'strict-transport-security': 'max-age=63072000'
    }
  }
}
[R/T] ✘ Check state.errors.job-1 for details.
[CLI] ✔ State written to tmp/errors/output.json
[CLI] ⚠ Errors reported in 1 jobs
[CLI] ✔ Finished in 860ms

Reference errors

image

// eg, if this came from our code, it doesn't help the user to see it but it does help us!
// @ts-ignore
this[util.inspect.custom] = (_depth, _options, _inspect) => {
const str = `[${this.name}] ${this.message}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This turned out to be a major cause of information getting lost :(

@josephjclark
Copy link
Collaborator Author

josephjclark commented Jul 2, 2024

TODO: a little worker testing locally, and running through a couple more cases in the issues. But I think this is about ready.

EDIT: Glad I did this, caught a bug in logging for the worker. All fixed now, errors look great. Lets check CI then I think I can version up

@josephjclark josephjclark marked this pull request as ready for review July 3, 2024 15:26
@josephjclark josephjclark merged commit a3cbb38 into main Jul 5, 2024
6 checks passed
@josephjclark josephjclark deleted the better-error-serializing branch July 5, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Error objects don't log well
1 participant