Add Detailed Execution Error Reporting

natjoe4 commented 1 year ago

Pull requests into cql-execution require the following. Submitter and reviewer should ✔ when done. For items that are not-applicable, mark "N/A" and ✔.

CDS Connect and Bonnie are the main users of this repository. It is strongly recommended to include a person from each of those projects as a reviewer.

Summary

Errors thrown during execution will now be throw as AnnotatedErrors, providing the Expression name, ELM local id, source Library identifier and version, as well as the initial error message. These changes aim to help faster identify where and why issues during execution arise.

Code Changes

Added try/catch workflow to base Expression class on execute function that catches errors and throws AnnotatedErrors with additional execution failure information.

Submitter:

[x] This pull request describes why these changes were made
[x] Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
[x] Tests are included and test edge cases
[x] Tests have been run locally and pass
[x] Code coverage has not gone down and all code touched or added is covered.
[x] Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
[x] All dependent libraries are appropriately updated or have a corresponding PR related to this change
[x] cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

[ ] Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
[ ] The tests appropriately test the new code, including edge cases
[ ] You have tried to break the code

codecov-commenter commented 1 year ago

Codecov Report

Merging #308 (7b05e4e) into master (e528df3) will increase coverage by 0.00%. The diff coverage is 89.28%.

:exclamation: Current head 7b05e4e differs from pull request most recent head 98ab592. Consider uploading reports for the commit 98ab592 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master     #308   +/-   ##
=======================================
  Coverage   86.24%   86.24%           
=======================================
  Files          50       51    +1     
  Lines        4398     4421   +23     
  Branches     1233     1241    +8     
=======================================
+ Hits         3793     3813   +20     
- Misses        319      320    +1     
- Partials      286      288    +2

Impacted Files	Coverage Δ
src/elm/expression.ts	`75.67% <82.35%> (-0.33%)`	:arrow_down:
src/cql.ts	`100.00% <100.00%> (ø)`
src/util/customErrors.ts	`100.00% <100.00%> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmoesel commented 1 year ago

I noticed that the stack trace always points to the execution function in Expression -- which isn't terribly helpful. I went down a bit of a rabbit hole because TS would not recognize the cause property that I thought would be inherited from Error. In the end, it turns out that if you just throw on a cause property yourself, it works as needed.

results in:

That's probably not particularly helpful or meaningful to an end user -- but it might be helpful to us.

mgramigna commented 1 year ago

I noticed that the stack trace always points to the execution function in Expression -- which isn't terribly helpful. I went down a bit of a rabbit hole because TS would not recognize the cause property that I thought would be inherited from Error. In the end, it turns out that if you just throw on a cause property yourself, it works as needed.

Great find. Sounds like it's exactly what we'd want based on the scenario. From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause:

The cause data property of an Error instance indicates the specific original cause of the error.

It is used when catching and re-throwing an error with a more-specific or useful error message in order to still have access to the original error.

Now that the original error is passed in to the AnnotatedError constructor, is it even worth having an individual message parameter in that constructor now? All we initialize for that is e.message. I could keep it for flexibility, or just remove it entirely?

mgramigna commented 1 year ago

@cmoesel @hossenlopp Thanks for the detailed feedback! I believe everything above should be resolved. I'm happy with the current state of it -- let me know what you think!

mgramigna commented 1 year ago

Will we be merging into master and then rebasing to use on next?

@hossenlopp I think so yeah, shouldn't be too bad, might just need to use a slightly different assertion to test for promise rejection in one of the tests. But I think 2.X versions of the library would greatly benefit from this change as well

cqframework / cql-execution