[React RUM] Add a ReactComponentTracker component #3086
Conversation
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3086 +/- ##
==========================================
- Coverage 93.47% 93.40% -0.07%
==========================================
Files 287 288 +1
Lines 7784 7795 +11
Branches 1759 1760 +1
==========================================
+ Hits 7276 7281 +5
- Misses 508 514 +6 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
| @@ -0,0 +1,23 @@ | |||
| export function getTimer() { | |||
There was a problem hiding this comment.
💬 suggestion: rename this function to createTimer. The file could be renamed to timer.ts.
| const timer = getTimer() | ||
|
|
||
| timer.startTimer() | ||
| setTimeout(() => { | ||
| timer.stopTimer() | ||
| expect(timer.getDuration()).toBeGreaterThan(1000) | ||
| }, 1000) |
There was a problem hiding this comment.
💬 suggestion: we don't want to actually wait for one second in unit tests. You can use mockClock():
| const timer = getTimer() | |
| timer.startTimer() | |
| setTimeout(() => { | |
| timer.stopTimer() | |
| expect(timer.getDuration()).toBeGreaterThan(1000) | |
| }, 1000) | |
| const clock = mockClock() | |
| registerCleanupCallback(clock.cleanup) | |
| const timer = getTimer() | |
| timer.startTimer() | |
| clock.tick(1000) | |
| timer.stopTimer() | |
| expect(timer.getDuration()).toBe(1000) |
| let duration: number | ||
| let startTime: number |
There was a problem hiding this comment.
💬 suggestion: those types are weak as they might be undefined when used.
| let duration: number | |
| let startTime: number | |
| let duration: number | undefined | |
| let startTime: number | undefined |
| } | ||
|
|
||
| function getDuration() { | ||
| return duration ? duration : 0 |
There was a problem hiding this comment.
💬 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 undefined instead?
| return duration ? duration : 0 | |
| return duration |
| isFirstRender: isFirstRender.current, | ||
| renderPhaseDuration: renderDuration, | ||
| effectPhaseDuration: effectDuration, | ||
| layoutEffectPhaseDuration: layoutEffectDuration, | ||
| componentName, |
There was a problem hiding this comment.
💬 suggestion: properties are usually snake_cased by convention
| isFirstRender: isFirstRender.current, | |
| renderPhaseDuration: renderDuration, | |
| effectPhaseDuration: effectDuration, | |
| layoutEffectPhaseDuration: layoutEffectDuration, | |
| componentName, | |
| is_first_render: isFirstRender.current, | |
| render_phase_duration: renderDuration, | |
| effect_phase_duration: effectDuration, | |
| layout_effect_phase_duration: layoutEffectDuration, | |
| component_name: componentName, |
|
|
||
| const totalRenderTime = renderDuration + effectDuration + layoutEffectDuration | ||
|
|
||
| addDurationVital(`${componentName}`, { |
There was a problem hiding this comment.
🥜 nitpick: no need to cast this as a string:
| addDurationVital(`${componentName}`, { | |
| addDurationVital(componentName, { |
| /** | ||
| * Send a custom vital tracking this duration | ||
| */ |
There was a problem hiding this comment.
💬 suggestion: remove this comment?
| onRender={() => { | ||
| renderTimer.startTimer() | ||
| }} | ||
| onLayoutEffect={() => { | ||
| layoutEffectTimer.startTimer() | ||
| }} | ||
| onEffect={() => { | ||
| effectTimer.startTimer() | ||
| }} | ||
| /> | ||
| {children} | ||
| <LifeCycle | ||
| onRender={() => { | ||
| renderTimer.stopTimer() | ||
| }} | ||
| onLayoutEffect={() => { | ||
| layoutEffectTimer.stopTimer() | ||
| }} |
There was a problem hiding this comment.
💬 suggestion:
| onRender={() => { | |
| renderTimer.startTimer() | |
| }} | |
| onLayoutEffect={() => { | |
| layoutEffectTimer.startTimer() | |
| }} | |
| onEffect={() => { | |
| effectTimer.startTimer() | |
| }} | |
| /> | |
| {children} | |
| <LifeCycle | |
| onRender={() => { | |
| renderTimer.stopTimer() | |
| }} | |
| onLayoutEffect={() => { | |
| layoutEffectTimer.stopTimer() | |
| }} | |
| onRender={renderTimer.startTimer} | |
| onLayoutEffect={layoutEffectTimer.startTimer} | |
| onEffect={effectTimer.startTimer} | |
| /> | |
| {children} | |
| <LifeCycle | |
| onRender={renderTimer.stopTimer} | |
| onLayoutEffect={layoutEffectTimer.stopTimer} |
| import { getTimer } from './getTimer' | ||
| import { addDurationVital } from './addDurationVital' | ||
|
|
||
| export const ReactComponentTracker = ({ |
There was a problem hiding this comment.
💬 suggestion: rename this as UNSTABLE_ReactComponentTracker
| } | ||
|
|
||
| function stopTimer() { | ||
| duration = performance.timeOrigin + performance.now() - startTime |
There was a problem hiding this comment.
💭 thought: I was considering to suggest using functions from timeUtils (ex: timeStampNow() etc), similarly to the rest of the SDK including Duration Vitals. But then we would use Date.now() instead of performance.now(), which is somewhat less precise. So using performance.now() here might make sense, but then why not use it in Vitals?
🤔 🤔
There was a problem hiding this comment.
💬 suggestion: add some unit test for that component
Miz85
force-pushed
the
nazim/react-timings
branch
from
5d48861 to
718555e
Compare
Motivation
Provide a way to measure React components render duration and report them to DataDog as
Custom VitalsChanges
performancedomain in therum-reactpackageReactComponentTrackercomponent that can be used to wrap components that you want to measure render duration for.Testing
I have gone over the contributing documentation.