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

fix(kernel): stack preserving error logging for console #1884

Merged
merged 7 commits into from
Jan 26, 2024

Conversation

Sayan751
Copy link
Contributor

Pull Request

πŸ“– Description

As of now, the console log for the following code produced incorrect/obfuscated stack information.

try {
  throw new Error('foo');
} catch (ex) {
  this.logger.error(ex);
}

It produced the following stack information:

2024-01-25T19:59:55.854Z [ERR my-el] Error: foo
emit                  @ index.mjs:1509
K                     @ index.mjs:1633
error                 @ index.mjs:1613
fn                    @ my-app.ts:22
attached              @ my-app.ts:14
Ot                    @ index.mjs:4312
$t                    @ index.mjs:4146
bind                  @ index.mjs:4071
activate              @ index.mjs:4035
$t                    @ index.mjs:4143
bind                  @ index.mjs:4071
activate              @ index.mjs:4035
(anonymous)           @ index.mjs:4744
onResolve             @ index.mjs:257
(anonymous)           @ index.mjs:4744
onResolve             @ index.mjs:257
activate              @ index.mjs:4744
(anonymous)           @ index.mjs:4858
onResolve             @ index.mjs:257
start                 @ index.mjs:4854
(anonymous)           @ main.ts:12
./src/main.ts         @ main.ts:13
__webpack_require__   @ bootstrap:24
(anonymous)           @ startup:6
(anonymous)           @ startup:6

Friendlier would be something like below (result of directly logging the error using console.error).

2024-01-25T19:59:55.854Z [ERR my-el] Error: foo
  at MyEl.fn (my-app.ts:18:1)
  at MyEl.attached (my-app.ts:14:1)
  at Controller.Ot (index.mjs:4312:1)
  at Controller.$t (index.mjs:4146:1)
  at Controller.bind (index.mjs:4071:1)
  at Controller.activate (index.mjs:4035:1)
  at Controller.$t (index.mjs:4143:1)
  at Controller.bind (index.mjs:4071:1)
  at Controller.activate (index.mjs:4035:1)
  at index.mjs:4744:1
emit                  @ index.mjs:1509
K                     @ index.mjs:1633
error                 @ index.mjs:1613
fn                    @ my-app.ts:22
attached              @ my-app.ts:14
Ot                    @ index.mjs:4312
$t                    @ index.mjs:4146
bind                  @ index.mjs:4071
activate              @ index.mjs:4035
$t                    @ index.mjs:4143
bind                  @ index.mjs:4071
activate              @ index.mjs:4035
(anonymous)           @ index.mjs:4744
onResolve             @ index.mjs:257
(anonymous)           @ index.mjs:4744
onResolve             @ index.mjs:257
activate              @ index.mjs:4744
(anonymous)           @ index.mjs:4858
onResolve             @ index.mjs:257
start                 @ index.mjs:4854
(anonymous)           @ main.ts:12
./src/main.ts         @ main.ts:13
__webpack_require__   @ bootstrap:24
(anonymous)           @ startup:6
(anonymous)           @ startup:6

Reproduction: https://stackblitz.com/edit/logging-and-stacktrace?file=src%2Fmy-app.ts,src%2Fmain.ts

This PR fixes this issue.

🎫 Issues

πŸ‘©β€πŸ’» Reviewer Notes

πŸ“‘ Test Plan

⏭ Next Steps

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (010a4ad) 88.55% compared to head (6603da1) 88.79%.
Report is 77 commits behind head on master.

Files Patch % Lines
packages/runtime-html/src/templating/controller.ts 76.71% 17 Missing ⚠️
packages/runtime/src/binding/ast.visitor.ts 78.12% 7 Missing ⚠️
packages/router-lite/src/route-expression.ts 33.33% 6 Missing ⚠️
packages/runtime/src/binding/ast.eval.ts 90.00% 5 Missing ⚠️
packages/platform/src/index.ts 81.81% 4 Missing ⚠️
.../router/src/instructions/instruction-parameters.ts 81.81% 2 Missing ⚠️
packages/compat-v1/src/compat-delegate.ts 50.00% 1 Missing ⚠️
packages/i18n/src/t/translation-binding.ts 88.88% 1 Missing ⚠️
packages/route-recognizer/src/index.ts 98.07% 1 Missing ⚠️
...ges/runtime-html/src/compiler/template-compiler.ts 95.65% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1884      +/-   ##
==========================================
+ Coverage   88.55%   88.79%   +0.23%     
==========================================
  Files         248      247       -1     
  Lines       22290    22317      +27     
  Branches     5180     5186       +6     
==========================================
+ Hits        19739    19816      +77     
+ Misses       2551     2501      -50     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@Sayan751 Sayan751 requested a review from bigopon January 25, 2024 20:36
@Sayan751 Sayan751 marked this pull request as ready for review January 25, 2024 20:36
Copy link
Member

@bigopon bigopon left a comment

Choose a reason for hiding this comment

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

Nice work @Sayan751 . I dont think we need to change the message property name. Though if you insist we can discuss more on that

package.json Outdated
@@ -113,7 +113,7 @@
"@typescript-eslint/eslint-plugin": "6.10.0",
"@typescript-eslint/parser": "6.10.0",
"chalk": "4.1.2",
"chromedriver": "119.0.0",
"chromedriver": "121.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

πŸ‘

@@ -226,8 +226,13 @@ export const format = toLookup({

export interface ILogEvent {
readonly severity: LogLevel;
readonly messageOrError: string | Error;
Copy link
Member

Choose a reason for hiding this comment

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

it should still be named message I think. It can be a plain (string) or packed (Error) message

@bigopon bigopon merged commit 030bfa1 into master Jan 26, 2024
26 checks passed
@bigopon bigopon deleted the topic/error-logging branch January 26, 2024 02:20
@Sayan751
Copy link
Contributor Author

Thanks @bigopon!

AureliaEffect pushed a commit that referenced this pull request Jan 26, 2024
2.0.0-beta.10 (2024-01-26)

**BREAKING CHANGE:**

* **enums:** string literal types replace const enums (#1870) ([e21e0c9](e21e0c9))

**Features:**

* **route-recognizer:** support for route parameter constraints (#1862) ([8f29cfd](8f29cfd))

**Bug Fixes:**

* **docs:** various doc fix
* **au-slot:** properly handle nested projection registration (#1881) ([00e8dee](00e8dee))
* **kernel:** stack preserving error logging for console (#1884) ([030bfa1](030bfa1))
* **portal:** remove target marker when deactivated (#1883) ([3db4c17](3db4c17))
* **validation:** evaluation of tagged rules from bindings (#1878) ([43d12f6](43d12f6))
* **validation:** property parsing with lambda and istanbul (#1877) ([71f82cf](71f82cf))
* **router:** store root/default page instruction correctly (#1869) ([84e6380](84e6380))
* **runtime-html:** template wrapping  (#1875) ([bfdaa3b](bfdaa3b))
* **i18n:** handle change of key in t.bind (#1868) ([c185764](c185764))
* **router-lite:** Router injection and ignoring null/undefined values for query parameters (#1859) ([6a79bc9](6a79bc9))
* **router-lite:** injection of Router alias ([6a79bc9](6a79bc9))
* **runtime-html:** fix broken tests ([bfdaa3b](bfdaa3b))
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

2 participants