-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we probably don't need to update docs! https://github.com/getsentry/sentry-docs/blob/master/src/platforms/javascript/guides/react/components/profiler.mdx
Perfect! 😄 |
@@ -34,6 +34,7 @@ class Profiler extends React.Component<ProfilerProps> { | |||
* Made protected for the React Native SDK to access | |||
*/ | |||
protected _mountSpan: Span | undefined = undefined; | |||
protected _updateSpan: Span | undefined = undefined; |
There was a problem hiding this comment.
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?
// 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), | ||
}); |
There was a problem hiding this comment.
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?
@@ -76,8 +79,6 @@ class Profiler extends React.Component<ProfilerProps> { | |||
// 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. |
There was a problem hiding this comment.
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
This PR gives the React SDK the ability to track the duration of
ui.react.update
spans by callingstartChild
within theshouldComponentUpdate
lifecycle method, and then callingfinish
incomponentDidUpdate
This will allow Performance users to see in their transactions how long it is taking their components to update
Before
After