Skip to content

feat(react): Track duration of React component updates #5531

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

Merged
merged 7 commits into from
Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
22 changes: 16 additions & 6 deletions packages/react/src/profiler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class Profiler extends React.Component<ProfilerProps> {
* Made protected for the React Native SDK to access
*/
protected _mountSpan: Span | undefined = undefined;
/**
* The span that represents the duration of time between shouldComponentUpdate and componentDidUpdate
*/
protected _updateSpan: Span | undefined = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a doc string for this?


// eslint-disable-next-line @typescript-eslint/member-ordering
public static defaultProps: Partial<ProfilerProps> = {
Expand Down Expand Up @@ -66,29 +70,35 @@ class Profiler extends React.Component<ProfilerProps> {
}
}

public componentDidUpdate({ updateProps, includeUpdates = true }: ProfilerProps): void {
// Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan,
public shouldComponentUpdate({ updateProps, includeUpdates = true }: ProfilerProps): boolean {
// Only generate an update span if includeUpdates is true, if there is a valid mountSpan,
// and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive.
// We are just trying to give baseline clues for further investigation.
if (includeUpdates && this._mountSpan && updateProps !== this.props.updateProps) {
// See what props haved changed between the previous props, and the current props. This is
// set as data on the span. We just store the prop keys as the values could be potenially very large.
const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]);
if (changedProps.length > 0) {
// The update span is a point in time span with 0 duration, just signifying that the component
// has been updated.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this comment since it's no longer true

const now = timestampWithMs();
this._mountSpan.startChild({
this._updateSpan = this._mountSpan.startChild({
data: {
changedProps,
},
description: `<${this.props.name}>`,
endTimestamp: now,
op: REACT_UPDATE_OP,
startTimestamp: now,
});
}
}

return true;
}

public componentDidUpdate(): void {
if (this._updateSpan) {
this._updateSpan.finish();
this._updateSpan = undefined;
}
}

// If a component is unmounted, we can say it is no longer on the screen.
Expand Down
12 changes: 8 additions & 4 deletions packages/react/test/profiler.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ jest.mock('@sentry/browser', () => ({

beforeEach(() => {
mockStartChild.mockClear();
mockFinish.mockClear();
activeTransaction = new MockSpan({ op: 'pageload' });
});

Expand Down Expand Up @@ -100,7 +101,9 @@ describe('withProfiler', () => {
});

it('is not created if hasRenderSpan is false', () => {
const ProfiledComponent = withProfiler(() => <h1>Testing</h1>, { includeRender: false });
const ProfiledComponent = withProfiler(() => <h1>Testing</h1>, {
includeRender: false,
});
expect(mockStartChild).toHaveBeenCalledTimes(0);

const component = render(<ProfiledComponent />);
Expand All @@ -115,32 +118,33 @@ describe('withProfiler', () => {
const ProfiledComponent = withProfiler((props: { num: number }) => <div>{props.num}</div>);
const { rerender } = render(<ProfiledComponent num={0} />);
expect(mockStartChild).toHaveBeenCalledTimes(1);
expect(mockFinish).toHaveBeenCalledTimes(1);

// Dispatch new props
rerender(<ProfiledComponent num={1} />);
expect(mockStartChild).toHaveBeenCalledTimes(2);
expect(mockStartChild).toHaveBeenLastCalledWith({
data: { changedProps: ['num'] },
description: `<${UNKNOWN_COMPONENT}>`,
endTimestamp: expect.any(Number),
op: REACT_UPDATE_OP,
startTimestamp: expect.any(Number),
});

expect(mockFinish).toHaveBeenCalledTimes(2);
// New props yet again
rerender(<ProfiledComponent num={2} />);
expect(mockStartChild).toHaveBeenCalledTimes(3);
expect(mockStartChild).toHaveBeenLastCalledWith({
data: { changedProps: ['num'] },
description: `<${UNKNOWN_COMPONENT}>`,
endTimestamp: expect.any(Number),
op: REACT_UPDATE_OP,
startTimestamp: expect.any(Number),
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we assert on mockFinish after this block here?

expect(mockFinish).toHaveBeenCalledTimes(3);

// Should not create spans if props haven't changed
rerender(<ProfiledComponent num={2} />);
expect(mockStartChild).toHaveBeenCalledTimes(3);
expect(mockFinish).toHaveBeenCalledTimes(3);
});

it('does not get created if hasUpdateSpan is false', () => {
Expand Down