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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Sep 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2172be8
feat(redux): Add 'attachReduxState' option
malay44 Sep 6, 2023
2b3735a
Merge branch 'getsentry:develop' into feature/redux-state-attachment
malay44 Sep 6, 2023
cd754fa
fix(redux): Improve Redux state attachment handling
malay44 Sep 6, 2023
4a7e091
test(redux): Add test case for Redux state attachment
malay44 Sep 7, 2023
6df9fb4
test(redux): Enhance Redux state attachment tests
malay44 Sep 8, 2023
8a25268
Update ReduxStateContext
malay44 Sep 11, 2023
dadf492
perf(redux): Add error filtering and bundle size optimization.
malay44 Sep 11, 2023
578da07
test(redux): Add test for undefined state and event type.
malay44 Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 24 additions & 1 deletion packages/react/src/redux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { configureScope, getCurrentHub } from '@sentry/browser';
import { addGlobalEventProcessor, configureScope, getCurrentHub } from '@sentry/browser';
import type { Scope } from '@sentry/types';
import { addNonEnumerableProperty } from '@sentry/utils';

Expand Down Expand Up @@ -49,6 +49,12 @@ type StoreEnhancerStoreCreator<Ext = Record<string, unknown>, StateExt = never>
) => Store<ExtendState<S, StateExt>, A, StateExt, Ext> & Ext;

export interface SentryEnhancerOptions<S = any> {
/**
* Redux state in attachments or not.
* @default true
*/
attachReduxState?: boolean;

/**
* Transforms the state before attaching it to an event.
* Use this to remove any private data before sending it to Sentry.
Expand All @@ -71,6 +77,7 @@ const ACTION_BREADCRUMB_CATEGORY = 'redux.action';
const ACTION_BREADCRUMB_TYPE = 'info';

const defaultOptions: SentryEnhancerOptions = {
attachReduxState: true,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

actionTransformer: action => action,
stateTransformer: state => state || null,
};
Expand All @@ -89,6 +96,22 @@ function createReduxEnhancer(enhancerOptions?: Partial<SentryEnhancerOptions>):

return (next: StoreEnhancerStoreCreator): StoreEnhancerStoreCreator =>
<S = any, A extends Action = AnyAction>(reducer: Reducer<S, A>, initialState?: PreloadedState<S>) => {
options.attachReduxState &&
addGlobalEventProcessor((event, hint) => {
if (
event.contexts &&
event.contexts.state &&
event.contexts.state.state &&
event.contexts.state.state.type === 'redux'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'

Copy link
Member

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

) {
hint.attachments = [
...(hint.attachments || []),
{ filename: 'redux_state.json', data: JSON.stringify(event.contexts.state.state.value) },
];
}
return event;
});

const sentryReducer: Reducer<S, A> = (state, action): S => {
const newState = reducer(state, action);

Expand Down
9 changes: 9 additions & 0 deletions packages/types/src/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ export interface Contexts extends Record<string, Context | undefined> {
response?: ResponseContext;
trace?: TraceContext;
cloud_resource?: CloudResourceContext;
state?: ReduxStateContext;
}

export interface ReduxStateContext extends Record<string, unknown> {
state: {
[key: string]: any;
type: string;
value: any;
};
malay44 marked this conversation as resolved.
Show resolved Hide resolved
}

export interface AppContext extends Record<string, unknown> {
Expand Down