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: some error aren't logged at all! #855

Closed
josephjclark opened this issue Jan 14, 2025 · 1 comment · Fixed by #856
Closed

Runtime: some error aren't logged at all! #855

josephjclark opened this issue Jan 14, 2025 · 1 comment · Fixed by #856
Assignees

Comments

@josephjclark
Copy link
Collaborator

Found a bit of a strange case in the new error handler (a shame we couldn't find this in QA)

I think if adaptor (or job?) code throws new Error(), we don't log it very nicely. Error details aren't logged, and there is no positional information.

For example:

[CLI] ♦ Versions:
         ▸ node.js                    18.17.1
         ▸ cli                        1.10.0
         ▸ @openfn/language-common    monorepo
[CLI] ✔ Loading adaptors from monorepo at /home/joe/repo/openfn/adaptors
[CLI] ⚠ Skipping auto-install as monorepo is being used
[CLI] ✔ Compiled all expressions in workflow
[R/T] ✘ job-1 aborted with error (130ms)

[R/T] ✘ TEST_ERROR: this is a test
[R/T] ✘ Check state.errors.job-1 for details
[CLI] ✔ State written to tmp/output.json
[CLI] ⚠ Errors reported in 1 jobs
[CLI] ✔ Finished in 199ms

The test error there should have a position, and should log details (key/value pairs)

You can repro by adding this to common:

export function tmp() {
  return state => {
    throwError('TEST_ERROR', { description: 'this is a test', jam: 'jar' });
  };
}
@josephjclark josephjclark self-assigned this Jan 14, 2025
@josephjclark josephjclark added this to v2 Jan 14, 2025
@github-project-automation github-project-automation bot moved this to New Issues in v2 Jan 14, 2025
@josephjclark josephjclark moved this from New Issues to In progress in v2 Jan 14, 2025
@josephjclark
Copy link
Collaborator Author

Ah I think the problem here is when we're running from the monorepo!

basically what it is is:

  • when an error is thrown, we look at the callstack
  • we try and work out which frames were thrown by an adaptor
  • if we detect an error from an adaptor, we do all kinds of nice error handling

Usually we identify an adaptor because it has @openfn/languages- in the path. But that's not true in the monorepo, when the path is more like: /home/joe/repo/openfn/adaptors/packages/common/dist/index.cjs:404:13

Error: TEST_ERROR: this is a test
    at throw_error_default (/home/joe/repo/openfn/adaptors/packages/common/dist/index.cjs:404:13)
    at /home/joe/repo/openfn/adaptors/packages/common/dist/index.cjs:573:5
    at file:///home/joe/repo/openfn/kit/packages/runtime/dist/index.js:649:22
    at async file:///home/joe/repo/openfn/kit/packages/runtime/dist/index.js:614:22 {
  code: 'TEST_ERROR',
  description: 'this is a test',
  jam: 'jar'
}

One thing I notice is that the adaptor paths don't start with file://. Is this something to do with my local dev environment, or is there really a pattern there? Is that giving us a hint that the error is coming from inside the vm?

@github-project-automation github-project-automation bot moved this from In progress to Done in v2 Jan 14, 2025
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 a pull request may close this issue.

1 participant