ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

onurtemizkan · 2021-07-27T13:35:46Z

(Edited by @Lms24 because the PR changed quite a bit)

Previously, exceptions caught in the Sentry Angular ErrorHandler defaulted to the generic mechanism, meaning they were marked as handled: true and mechanism: generic. This PR adds a custom mechanism for these events: handled: false (because they were caught in our ErrorHandler) and mechanism: angular.
The mechanism is set by using a Scope Eventprocessor (similarly to how we do it in the Remix SDK)

github-actions · 2021-07-27T13:48:51Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	22.37 KB (+0.05% 🔺)
@sentry/browser - Webpack	23.25 KB (+0.06% 🔺)
@sentry/react - Webpack	23.28 KB (+0.06% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	29.81 KB (+0.04% 🔺)

kamilogorek

The implementation is valid, so +1 on this. However, I think we should decide how to make a clear distinctions between handled/unhandled and mechanisms name. If we decide to go with this PR, then we should also update Vue, Ember, React and all remaining frameworks to with the correct mechanism name.

cc @getsentry/team-webplatform

lobsterkatie · 2021-08-05T22:28:20Z

I think we should decide how to make a clear distinctions between handled/unhandled and mechanisms name.

I've been wanting to circle back around to mechanism for a while. I definitely think there are ways we could be clearer about what's what.

rhcarvalho

@AbhiPrasad pointed me here when we were talking about hints.

Hints work, but are a form of two-way communication between the SDK and user code that we sometimes abuse to pass information around. In particular, I find this use of hint.data harder to understand and maintain than alternatives.

One alternative, what SDKs like angular, ember, etc can do is to use a Scope-based Event Processor to mutate the event in arbitrary ways, without requiring extra code in upstream packages like @sentry/browser (so probably a win in terms of bundle size).

HazAT · 2021-09-06T11:16:36Z

These changes lgtm, can we merge it?

github-actions · 2021-10-14T12:01:50Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

bruno-garcia · 2021-11-28T23:30:05Z

@onurtemizkan could you please update this branch and lets merge?

This resolve an issue I opened (#3836) and seems to have all approvals it needs

AbhiPrasad · 2021-11-29T15:23:46Z

I would like to see us switch to using a scope processor vs. adding the mechanism through a hint.

github-actions · 2022-07-12T10:59:59Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.43 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.06 KB (+0.05% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.01 KB (+0.05% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	52.97 KB (+0.06% 🔺)
@sentry/browser - Webpack (gzipped + minified)	19.77 KB (+0.05% 🔺)
@sentry/browser - Webpack (minified)	64.31 KB (+0.05% 🔺)
@sentry/react - Webpack (gzipped + minified)	19.79 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.08 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.88 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.13 KB (+0.05% 🔺)

Lms24

@onurtemizkan is there any chance we could add a test for this change? I recently started adding tests for the changes I made to the Angular SDK and I think it should be possible to add a simple test for this.
I know that our testing setup is far from optimal for Angular rn but it's on my todo list to change that.

If it turns out that it's not that easy then that's fine as well. We can come back to it once I improved our setup.

EDIT: fyi, in #5416 I'm adding a few more tests.

Lms24

One more remark @onurtemizkan, thanks!

Lms24 · 2022-07-14T14:55:05Z

packages/angular/src/errorhandler.ts

+      getCurrentHub().captureException(extractedError, { data: { mechanism: { type: 'angular', handled: false } } }),
+    );


Can we change the way of passing on the mechanism to using an eventprocessor on the scope? Similarly to how we do it in remix here? If this is possible, we'd prefer that over adding it to the hint.

- Provide 'angular' as the mechanism for handled angular errors.

onurtemizkan · 2022-07-20T15:23:44Z

Thanks for the review @Lms24!

Updated the PR

Lms24 · 2022-07-20T15:47:57Z

packages/browser/src/eventbuilder.ts

+  const providedMechanism: Mechanism | undefined =
+    hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism;
+  const mechanism: Mechanism = providedMechanism || {
+    handled: true,
+    type: 'generic',
+  };
+
+  addExceptionMechanism(event, mechanism);
+


Now that we're using the scope eventprocessor - do we still need this?

The default fallback mechanism seems unneeded at this point, removing it.

Not sure about the rest, though.
The original intent on this PR was to mimic how mechanisms are passed in Node SDK on browser SDK, but a lot changed since, so I don't know if that's still relevant.

Still, it looks like this can also be moved into scope.addEventProcessor to be consistent, if we are keeping it.

I think it's better to change the Node approach to how we do it in Express (and Angular). (But let's do this in another PR). Which makes me think that we don't need to query the hint here. The original call to addExceptionMechanism shouldn't be a problem because it won't overwrite the already set mechanism with the default value (see here)

So overall, I don't think we need this change here (unless I'm missing something)

Lms24

Thanks for adding the tests and the scope event processor @onurtemizkan
The linter isn't happy yet but I think that's easily fixable.

Just one more question (above) and then I think this is ready to go.

Lms24

Thanks @onurtemizkan ❤️

Lms24 · 2022-07-22T07:45:50Z

Tested on NG14 test project:

onurtemizkan requested a review from kamilogorek as a code owner July 27, 2021 13:35

kamilogorek approved these changes Jul 29, 2021

View reviewed changes

rhcarvalho reviewed Aug 11, 2021

View reviewed changes

github-actions bot added the Status: Stale label Oct 14, 2021

onurtemizkan added Status: In Progress and removed Status: Stale labels Oct 25, 2021

kamilogorek requested review from kamilogorek and removed request for kamilogorek January 10, 2022 14:08

bruno-garcia mentioned this pull request Jul 11, 2022

Angular error handler should report handled: false #3836

Closed

onurtemizkan force-pushed the onur/browser-provide-mechanism branch from 573feac to 039919d Compare July 12, 2022 10:52

Lms24 reviewed Jul 13, 2022

View reviewed changes

Lms24 assigned onurtemizkan Jul 13, 2022

Lms24 reviewed Jul 14, 2022

View reviewed changes

onurtemizkan added 3 commits July 20, 2022 13:11

feat(browser): Allow mechanism to be provided as a hint.

438e74f

- Provide 'angular' as the mechanism for handled angular errors.

Use event processor to pass mechanism.

755a519

Add errorhandler tests.

5d94e2a

onurtemizkan force-pushed the onur/browser-provide-mechanism branch from 039919d to 5d94e2a Compare July 20, 2022 15:23

onurtemizkan requested a review from Lms24 July 20, 2022 15:24

Lms24 reviewed Jul 20, 2022

View reviewed changes

Fix linter.

914aa56

onurtemizkan added 3 commits July 20, 2022 17:14

Remove unneeded default fallback mechanism.

bd1944d

Fix linter

e8b9a86

Remove changes on eventbuilder.

53a1d8b

Lms24 approved these changes Jul 21, 2022

View reviewed changes

Lms24 merged commit 3665831 into master Jul 22, 2022

Lms24 deleted the onur/browser-provide-mechanism branch July 22, 2022 07:53

Lms24 changed the title ~~feat(browser): Allow mechanism to be provided as a hint. Update angular exception mechanism.~~ ref(angular): Set Angular ErrorHandler specific Exception Mechanism Jul 22, 2022

Lms24 removed the Status: In Progress label Jul 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

onurtemizkan commented Jul 27, 2021 •

edited by Lms24

github-actions bot commented Jul 27, 2021 •

edited

kamilogorek left a comment

lobsterkatie commented Aug 5, 2021

rhcarvalho left a comment

HazAT commented Sep 6, 2021

github-actions bot commented Oct 14, 2021

bruno-garcia commented Nov 28, 2021

AbhiPrasad commented Nov 29, 2021

github-actions bot commented Jul 12, 2022 •

edited

Lms24 left a comment •

edited

Lms24 left a comment

Lms24 Jul 14, 2022 •

edited

onurtemizkan commented Jul 20, 2022

Lms24 Jul 20, 2022

onurtemizkan Jul 20, 2022

Lms24 Jul 21, 2022

Lms24 left a comment •

edited

Lms24 left a comment

Lms24 commented Jul 22, 2022

		getCurrentHub().captureException(extractedError, { data: { mechanism: { type: 'angular', handled: false } } }),
		);

ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

Conversation

onurtemizkan commented Jul 27, 2021 • edited by Lms24

github-actions bot commented Jul 27, 2021 • edited

size-limit report

kamilogorek left a comment

Choose a reason for hiding this comment

lobsterkatie commented Aug 5, 2021

rhcarvalho left a comment

Choose a reason for hiding this comment

HazAT commented Sep 6, 2021

github-actions bot commented Oct 14, 2021

bruno-garcia commented Nov 28, 2021

AbhiPrasad commented Nov 29, 2021

github-actions bot commented Jul 12, 2022 • edited

size-limit report 📦

Lms24 left a comment • edited

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Jul 14, 2022 • edited

Choose a reason for hiding this comment

onurtemizkan commented Jul 20, 2022

Lms24 Jul 20, 2022

Choose a reason for hiding this comment

onurtemizkan Jul 20, 2022

Choose a reason for hiding this comment

Lms24 Jul 21, 2022

Choose a reason for hiding this comment

Lms24 left a comment • edited

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 commented Jul 22, 2022

onurtemizkan commented Jul 27, 2021 •

edited by Lms24

github-actions bot commented Jul 27, 2021 •

edited

github-actions bot commented Jul 12, 2022 •

edited

Lms24 left a comment •

edited

Lms24 Jul 14, 2022 •

edited

Lms24 left a comment •

edited