-
Notifications
You must be signed in to change notification settings - Fork 140
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
[React RUM] Add a ReactComponentTracker component #3086
base: main
Are you sure you want to change the base?
Changes from all commits
5407cc3
d1ca10e
131ef71
51dec02
eebe16b
f9325ae
19cb0a6
718555e
ea3b972
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import type { RumPublicApi } from '@datadog/browser-rum-core' | ||
import { onReactPluginInit } from '../reactPlugin' | ||
|
||
export const addDurationVital: RumPublicApi['addDurationVital'] = (name, options) => { | ||
onReactPluginInit((_, rumPublicApi) => { | ||
rumPublicApi.addDurationVital(name, options) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { getTimer } from './getTimer' | ||
|
||
describe('getTimer', () => { | ||
it('is able to measure time', () => { | ||
const timer = getTimer() | ||
|
||
timer.startTimer() | ||
setTimeout(() => { | ||
timer.stopTimer() | ||
expect(timer.getDuration()).toBeGreaterThan(1000) | ||
}, 1000) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,23 @@ | ||||||||||
export function getTimer() { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: rename this function to |
||||||||||
let duration: number | ||||||||||
let startTime: number | ||||||||||
Comment on lines
+2
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: those types are weak as they might be undefined when used.
Suggested change
|
||||||||||
|
||||||||||
function startTimer() { | ||||||||||
const start = performance.now() | ||||||||||
startTime = performance.timeOrigin + start | ||||||||||
} | ||||||||||
|
||||||||||
function stopTimer() { | ||||||||||
duration = performance.timeOrigin + performance.now() - startTime | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 thought: I was considering to suggest using functions from 🤔 🤔 |
||||||||||
} | ||||||||||
|
||||||||||
function getDuration() { | ||||||||||
return duration ? duration : 0 | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: I don't think duration should be 0 if the timer has not be stopped. If the timer is used incorrectly, we might report unexpected durations, and this could have an impact on data analysis. Maybe return
Suggested change
|
||||||||||
} | ||||||||||
|
||||||||||
function getStartTime() { | ||||||||||
return startTime | ||||||||||
} | ||||||||||
|
||||||||||
return { startTimer, stopTimer, getDuration, getStartTime } | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { ReactComponentTracker } from './reactComponentTracker' |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: add some unit test for that component |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,97 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
import * as React from 'react' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { getTimer } from './getTimer' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { addDurationVital } from './addDurationVital' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const ReactComponentTracker = ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: rename this as |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
name: componentName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
children, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
name: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
context?: object | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
children?: React.ReactNode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
burstDebounce?: number | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const isFirstRender = React.useRef(true) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const renderTimer = getTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const effectTimer = getTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const layoutEffectTimer = getTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const onEffectEnd = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const renderDuration = renderTimer.getDuration() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const effectDuration = effectTimer.getDuration() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const layoutEffectDuration = layoutEffectTimer.getDuration() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const totalRenderTime = renderDuration + effectDuration + layoutEffectDuration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
addDurationVital(`${componentName}`, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥜 nitpick: no need to cast this as a string:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
startTime: renderTimer.getStartTime(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
duration: totalRenderTime, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
context: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
isFirstRender: isFirstRender.current, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
renderPhaseDuration: renderDuration, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
effectPhaseDuration: effectDuration, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
layoutEffectPhaseDuration: layoutEffectDuration, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
componentName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+31
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: properties are usually snake_cased by convention
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
framework: 'react', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Send a custom vital tracking this duration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: remove this comment? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
isFirstRender.current = false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* In react, children are rendered sequentially | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* in the order they are defined. that's why we | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* can measure perf timings of a component by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* starting recordings in the component above | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* and stopping them in the component below. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
<> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
<LifeCycle | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onRender={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
renderTimer.startTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onLayoutEffect={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
layoutEffectTimer.startTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onEffect={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
effectTimer.startTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
{children} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
<LifeCycle | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onRender={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
renderTimer.stopTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onLayoutEffect={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
layoutEffectTimer.stopTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+57
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
onEffect={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
effectTimer.stopTimer() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onEffectEnd() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
</> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
function LifeCycle({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onRender, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onLayoutEffect, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onEffect, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onRender: () => void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onLayoutEffect: () => void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onEffect: () => void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
onRender() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
React.useLayoutEffect(onLayoutEffect) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
React.useEffect(onEffect) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export { ErrorBoundary, addReactError } from '../domain/error' | ||
export { reactPlugin } from '../domain/reactPlugin' | ||
export { ReactComponentTracker } from '../domain/performance' |
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.
💬 suggestion: we don't want to actually wait for one second in unit tests. You can use
mockClock()
: