feat(redux): Add 'attachReduxState' option #8953

malay44 · 2023-09-06T05:33:06Z

This pull request introduces a new configuration option, attachReduxState, with a default value of true. This option lets users control whether the Redux state should automatically attach to Sentry events.

Changes Made:

Added attachReduxState configuration option, default true.
Implemented attachments to store the Redux state when attachReduxState is set to true.
Added tests to verify that the attachment logic correctly attaches the Redux state when available, avoids it when it's empty or undefined, and verifies that it's of the correct type, redux.

Motivation:
By default, the 'attachReduxState' option is set to true, enabling the attachment of the Redux state to Sentry events. This enhancement improves error tracking for Redux-based applications.

Testing:
I have thoroughly tested these changes and confirmed that they work as intended.

Related Issues:
Closes GH-6266

Screenshots:

Introduces 'attachReduxState' (default: true) for controlling Redux state attachment to Sentry events. Fixes getsentryGH-6266

HazAT

This is awesome, looking forward merging this!
Maybe we can add a small test for this, should be too hard since we already have quite a few tests for Redux.

packages/react/src/redux.ts

malay44 · 2023-09-06T11:25:06Z

Thanks for the positive feedback! I'll add a test for this feature and let you know the progress. 🤗

This is awesome, looking forward merging this! Maybe we can add a small test for this, should be too hard since we already have quite a few tests for Redux.

packages/react/src/redux.ts

AbhiPrasad · 2023-09-06T12:11:26Z

packages/react/src/redux.ts

@@ -71,6 +77,7 @@ const ACTION_BREADCRUMB_CATEGORY = 'redux.action';
 const ACTION_BREADCRUMB_TYPE = 'info';

 const defaultOptions: SentryEnhancerOptions = {
+  attachReduxState: true,


I'm not sure we can default this to be true because attachments have their own quota. Thoughts @HazAT?

hmm I think it's fine - Redux while popular is not in every React app - and this feature is very helpful.

Alright let's after we get some tests in @malay44!

Refactors the code responsible for attaching the Redux state to Sentry events. The change enhances efficiency by avoiding the addition of empty JSON attachments when there is no Redux state available.

malay44 · 2023-09-07T08:00:34Z

packages/react/src/redux.ts

+            event.contexts &&
+            event.contexts.state &&
+            event.contexts.state.state &&
+            event.contexts.state.state.type === 'redux'


What if we add an extra condition, such as event instanceof ErrorEvent ? I noticed that when I was debugging in the browser, it was also attaching the Redux state when the event type was 'transaction'

yup we should do this.

to filter for just errors you can add event.type === undefined. The reason there is no error type is because of backwards compatibility 😢 (error is undefined, all other events have a type).

Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:

try { // @ts-expect-error try catch to reduce bundle size if (event.type === undefined && event.contexts.state.state.type === 'redux') { hint.attachments = [ ...(hint.attachments || []), // @ts-expect-error try catch to reduce bundle size { filename: 'redux_state.json', data: JSON.stringify(event.contexts.state.state.value) }, ]; } } catch (_) { // empty }

Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:

This article is fantastic! I've learned a lot while working on this issue. Thank you all so much for your guidance; I appreciate it.

I'm eager to take on more challenging issues. Please consider assigning some to me. I'm looking forward to learning and growing while contributing to the project.

New test cases to validate the attachment of Redux state to Sentry events. The test ensures that the attachment logic correctly adds the Redux state when available and does not add it when it's empty or undefined and when not of type redux.

AbhiPrasad

thanks for adding tests!

packages/react/test/reduxStateAttachments.test.ts

AbhiPrasad · 2023-09-07T12:14:00Z

packages/react/test/reduxStateAttachments.test.ts

+    Sentry.configureScope(scope => {
+      expect(scope.getAttachments()).toContainEqual(
+        expect.objectContaining({
+          filename: 'redux_state.json',
+          data: JSON.stringify({
+            value: 'updated',
+          }),
+        }),
+      );
+    });
+  });


We can access the attachments on the scope via Sentry.getCurrentHub().getScope().getAttachments() - let's do that for the rest of the uses of configureScope as well.

I discovered today that I made an error when writing the tests. They currently pass for every scenario. I'll investigate this issue further and let you know how I'm doing.

Refined the test suite for Redux state attachment by: 1. Removing an incorrect test from`reduxStateAttachments.test.ts`. 2. Adding a new test case to `redux.ts` to verify that the attachment logic correctly attaches the Redux state when available, avoids it when it's empty or undefined, and verifies that it's of the correct type 'redux'.

AbhiPrasad

let's make the redux state only attach for errors and add a test for this, and we are good to merge.

AbhiPrasad · 2023-09-11T15:52:51Z

packages/react/src/redux.ts

+            event.contexts &&
+            event.contexts.state &&
+            event.contexts.state.state &&
+            event.contexts.state.state.type === 'redux'


yup we should do this.

to filter for just errors you can add event.type === undefined. The reason there is no error type is because of backwards compatibility 😢 (error is undefined, all other events have a type).

Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:

try { // @ts-expect-error try catch to reduce bundle size if (event.type === undefined && event.contexts.state.state.type === 'redux') { hint.attachments = [ ...(hint.attachments || []), // @ts-expect-error try catch to reduce bundle size { filename: 'redux_state.json', data: JSON.stringify(event.contexts.state.state.value) }, ]; } } catch (_) { // empty }

packages/types/src/context.ts

stale

Co-authored-by: Abhijeet Prasad <devabhiprasad@gmail.com>

AbhiPrasad

Thanks @malay44!

malay44 and others added 2 commits September 6, 2023 10:36

feat(redux): Add 'attachReduxState' option

2172be8

Introduces 'attachReduxState' (default: true) for controlling Redux state attachment to Sentry events. Fixes getsentryGH-6266

Merge branch 'getsentry:develop' into feature/redux-state-attachment

2b3735a

lforst requested review from lforst and AbhiPrasad September 6, 2023 08:26

HazAT previously requested changes Sep 6, 2023

View reviewed changes

packages/react/src/redux.ts Outdated Show resolved Hide resolved

AbhiPrasad reviewed Sep 6, 2023

View reviewed changes

malay44 force-pushed the feature/redux-state-attachment branch 3 times, most recently from 35fa934 to 2b3735a Compare September 6, 2023 22:12

fix(redux): Improve Redux state attachment handling

cd754fa

Refactors the code responsible for attaching the Redux state to Sentry events. The change enhances efficiency by avoiding the addition of empty JSON attachments when there is no Redux state available.

malay44 requested a review from AbhiPrasad September 6, 2023 22:17

malay44 commented Sep 7, 2023

View reviewed changes

AbhiPrasad reviewed Sep 7, 2023

View reviewed changes

malay44 force-pushed the feature/redux-state-attachment branch from dff0eb2 to 4a7e091 Compare September 8, 2023 09:07

malay44 requested review from AbhiPrasad and HazAT September 8, 2023 09:17

AbhiPrasad reviewed Sep 11, 2023

View reviewed changes

malay44 and others added 3 commits September 11, 2023 21:34

Update ReduxStateContext

8a25268

Co-authored-by: Abhijeet Prasad <devabhiprasad@gmail.com>

perf(redux): Add error filtering and bundle size optimization.

dadf492

test(redux): Add test for undefined state and event type.

578da07

AbhiPrasad approved these changes Sep 11, 2023

View reviewed changes

AbhiPrasad enabled auto-merge (squash) September 11, 2023 18:52

AbhiPrasad merged commit 595e4e2 into getsentry:develop Sep 11, 2023
68 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redux): Add 'attachReduxState' option #8953

feat(redux): Add 'attachReduxState' option #8953

malay44 commented Sep 6, 2023 •

edited

HazAT left a comment

malay44 commented Sep 6, 2023

AbhiPrasad Sep 6, 2023

HazAT Sep 6, 2023

AbhiPrasad Sep 6, 2023

malay44 Sep 7, 2023

AbhiPrasad Sep 11, 2023

malay44 Sep 11, 2023

AbhiPrasad left a comment

AbhiPrasad Sep 7, 2023

This comment was marked as resolved.

malay44 Sep 8, 2023

AbhiPrasad left a comment

AbhiPrasad Sep 11, 2023

AbhiPrasad left a comment

feat(redux): Add 'attachReduxState' option #8953

feat(redux): Add 'attachReduxState' option #8953

Conversation

malay44 commented Sep 6, 2023 • edited

HazAT left a comment

Choose a reason for hiding this comment

malay44 commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

malay44 commented Sep 6, 2023 •

edited