Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion packages/react/src/redux.ts
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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;
});

Comment thread
malay44 marked this conversation as resolved.
const sentryReducer: Reducer<S, A> = (state, action): S => {
const newState = reducer(state, action);

Expand Down
105 changes: 105 additions & 0 deletions packages/react/test/reduxStateAttachments.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import * as Sentry from '@sentry/browser';
import * as Redux from 'redux';
Comment thread
malay44 marked this conversation as resolved.
Outdated

import { createReduxEnhancer } from '../src/redux';

const mockAddBreadcrumb = jest.fn();
const mockSetContext = jest.fn();

jest.mock('@sentry/browser', () => ({
...jest.requireActual('@sentry/browser'),
}));

afterEach(() => {
mockAddBreadcrumb.mockReset();
mockSetContext.mockReset();
});

describe('Redux State Attachments', () => {
it('attaches Redux state to Sentry scope', () => {
const enhancer = createReduxEnhancer();

const initialState = {
value: 'initial',
};

const store = Redux.createStore((state = initialState) => state, enhancer);

const updateAction = { type: 'UPDATE_VALUE', value: 'updated' };

store.dispatch(updateAction);

const error = new Error('test');
Sentry.captureException(error);

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

Choose a reason for hiding this comment

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

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.

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


it('does not attach when attachReduxState is false', () => {
const enhancer = createReduxEnhancer({ attachReduxState: false });

const initialState = {
value: 'initial',
};

const store = Redux.createStore((state = initialState) => state, enhancer);

const updateAction = { type: 'UPDATE_VALUE', value: 'updated' };

store.dispatch(updateAction);

const error = new Error('test');
Sentry.captureException(error);

Sentry.configureScope(scope => {
expect(scope.getAttachments()).not.toContainEqual(
expect.objectContaining({
filename: 'redux_state.json',
data: expect.anything(),
}),
);
});
});

it('does not attach when state.type is not redux', () => {
const enhancer = createReduxEnhancer();

const initialState = {
value: 'initial',
};

Redux.createStore((state = initialState) => state, enhancer);

Sentry.configureScope(scope => {
scope.setContext('state', {
state: {
type: 'not_redux',
value: {
value: 'updated',
},
},
});
});

const error = new Error('test');
Sentry.captureException(error);

Sentry.configureScope(scope => {
expect(scope.getAttachments()).not.toContainEqual(
expect.objectContaining({
filename: 'redux_state.json',
data: expect.anything(),
}),
);
});
});
});
9 changes: 9 additions & 0 deletions packages/types/src/context.ts
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;
};
Comment thread
malay44 marked this conversation as resolved.
Outdated
}

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