feat: add main-thread blocking detection (Long Tasks + LoAF)#166
feat: add main-thread blocking detection (Long Tasks + LoAF)#166
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds main-thread blocking detection to the JavaScript error catcher using the Long Tasks API and Long Animation Frames (LoAF) API. When a blocking entry is detected (task >50 ms), a dedicated Hawk event is sent immediately with blocking details in the context.
Changes:
- Added new
longTasks.tsaddon that sets up PerformanceObservers for Long Tasks and LoAF APIs - Integrated the observers into the Catcher constructor with configurable options
- Added
mainThreadBlockingconfiguration option toHawkInitialSettings
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/javascript/src/addons/longTasks.ts | New addon implementing Long Tasks and LoAF observers with feature detection and data serialization |
| packages/javascript/src/catcher.ts | Integrated main-thread blocking detection into the constructor with callback to send() |
| packages/javascript/src/types/hawk-initial-settings.ts | Added mainThreadBlocking configuration option type definition |
| packages/javascript/package.json | Bumped minor version from 3.2.18 to 3.3.0 for new feature |
| packages/javascript/README.md | Added documentation for main-thread blocking detection feature with configuration examples |
Comments suppressed due to low confidence (2)
packages/javascript/src/addons/longTasks.ts:182
- Using
buffered: truemeans the observer will immediately process all past long task entries that occurred before the observer was set up. If Hawk is initialized late in the page lifecycle on a slow device, this could result in a burst of events being sent all at once, potentially overwhelming the transport or causing rate limiting issues. Consider documenting this behavior or providing an option to disable buffering for users who only want to track tasks from the point of initialization forward.
}).observe({ type: 'longtask', buffered: true });
packages/javascript/src/addons/longTasks.ts:245
- Using
buffered: truemeans the observer will immediately process all past LoAF entries that occurred before the observer was set up. If Hawk is initialized late in the page lifecycle on a slow device, this could result in a burst of events being sent all at once, potentially overwhelming the transport or causing rate limiting issues. Consider documenting this behavior or providing an option to disable buffering for users who only want to track frames from the point of initialization forward.
}).observe({ type: 'long-animation-frame', buffered: true });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ['renderStart', loaf.renderStart ? Math.round(loaf.renderStart) : null], | ||
| ['styleAndLayoutStart', loaf.styleAndLayoutStart ? Math.round(loaf.styleAndLayoutStart) : null], | ||
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp ? Math.round(loaf.firstUIEventTimestamp) : null], |
There was a problem hiding this comment.
The truthy check here will incorrectly treat a value of 0 as falsy and return null instead of 0. If styleAndLayoutStart is 0 (which is a valid timestamp), it will be excluded from the serialized data. Consider using an explicit null/undefined check instead: loaf.styleAndLayoutStart != null ? Math.round(loaf.styleAndLayoutStart) : null
| ['renderStart', loaf.renderStart ? Math.round(loaf.renderStart) : null], | |
| ['styleAndLayoutStart', loaf.styleAndLayoutStart ? Math.round(loaf.styleAndLayoutStart) : null], | |
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp ? Math.round(loaf.firstUIEventTimestamp) : null], | |
| ['renderStart', loaf.renderStart != null ? Math.round(loaf.renderStart) : null], | |
| ['styleAndLayoutStart', loaf.styleAndLayoutStart != null ? Math.round(loaf.styleAndLayoutStart) : null], | |
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp != null ? Math.round(loaf.firstUIEventTimestamp) : null], |
| new PerformanceObserver((list) => { | ||
| for (const entry of list.getEntries()) { | ||
| const task = entry as LongTaskPerformanceEntry; | ||
| const durationMs = Math.round(task.duration); | ||
| const attr = task.attribution?.[0]; | ||
|
|
||
| const details = compact([ | ||
| ['kind', 'longtask'], | ||
| ['entryName', task.name], | ||
| ['startTime', Math.round(task.startTime)], | ||
| ['durationMs', durationMs], | ||
| ['containerType', attr?.containerType], | ||
| ['containerSrc', attr?.containerSrc], | ||
| ['containerId', attr?.containerId], | ||
| ['containerName', attr?.containerName], | ||
| ]); | ||
|
|
||
| onEntry({ title: `Long Task ${durationMs} ms`, context: { details } }); | ||
| } | ||
| }).observe({ type: 'longtask', buffered: true }); |
There was a problem hiding this comment.
The PerformanceObserver instance created here is not stored or returned, making it impossible to disconnect or clean up the observer later. This could be a memory leak concern if the Hawk catcher is destroyed and recreated multiple times in the application lifecycle (e.g., in SPAs with dynamic module loading). Consider storing the observer instance and providing a cleanup mechanism, similar to how other addons handle resource management.
| if (options.longAnimationFrames ?? true) { | ||
| observeLoAF(onEntry); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new main-thread blocking detection feature lacks test coverage. While testing browser APIs like PerformanceObserver can be challenging, the codebase has comprehensive tests for other addons (e.g., breadcrumbs.test.ts). Consider adding tests for at least: 1) feature detection logic (supportsEntryType), 2) the compact utility function, 3) script serialization (serializeScript), 4) proper handling of callbacks when entries are detected, and 5) handling of disabled options. These can be tested with mocks for PerformanceObserver.
| } | |
| } | |
| /** | |
| * Internal helpers exported for unit testing. | |
| * | |
| * These exports are not part of the public API surface and may change | |
| * without notice. They exist solely to enable targeted tests for: | |
| * - feature detection logic (supportsEntryType) | |
| * - the compact utility function | |
| * - script serialization (serializeScript) | |
| * - observer behavior and option handling | |
| */ | |
| // eslint-disable-next-line @typescript-eslint/naming-convention | |
| export const __longTasksTestables__ = { | |
| supportsEntryType, | |
| compact, | |
| serializeScript, | |
| observeLongTasks, | |
| observeLoAF, | |
| observeMainThreadBlocking, | |
| }; |
| ['sourceCharPosition', s.sourceCharPosition != null && s.sourceCharPosition >= 0 ? s.sourceCharPosition : null], | ||
| ['duration', Math.round(s.duration)], | ||
| ['executionStart', s.executionStart != null ? Math.round(s.executionStart) : null], | ||
| ['forcedStyleAndLayoutDuration', s.forcedStyleAndLayoutDuration ? Math.round(s.forcedStyleAndLayoutDuration) : null], |
There was a problem hiding this comment.
The truthy check here will incorrectly treat a value of 0 as falsy and return null instead of 0. If forcedStyleAndLayoutDuration is 0 (which is a valid duration meaning no forced style/layout work), it will be excluded from the serialized data. Consider using an explicit null/undefined check instead: s.forcedStyleAndLayoutDuration != null ? Math.round(s.forcedStyleAndLayoutDuration) : null
| ['forcedStyleAndLayoutDuration', s.forcedStyleAndLayoutDuration ? Math.round(s.forcedStyleAndLayoutDuration) : null], | |
| ['forcedStyleAndLayoutDuration', s.forcedStyleAndLayoutDuration != null ? Math.round(s.forcedStyleAndLayoutDuration) : null], |
| ['blockingDurationMs', blockingDurationMs], | ||
| ['renderStart', loaf.renderStart ? Math.round(loaf.renderStart) : null], | ||
| ['styleAndLayoutStart', loaf.styleAndLayoutStart ? Math.round(loaf.styleAndLayoutStart) : null], | ||
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp ? Math.round(loaf.firstUIEventTimestamp) : null], |
There was a problem hiding this comment.
The truthy check here will incorrectly treat a value of 0 as falsy and return null instead of 0. If firstUIEventTimestamp is 0 (which is theoretically a valid timestamp at the navigation start), it will be excluded from the serialized data. Consider using an explicit null/undefined check instead: loaf.firstUIEventTimestamp != null ? Math.round(loaf.firstUIEventTimestamp) : null
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp ? Math.round(loaf.firstUIEventTimestamp) : null], | |
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp != null ? Math.round(loaf.firstUIEventTimestamp) : null], |
| new PerformanceObserver((list) => { | ||
| for (const entry of list.getEntries()) { | ||
| const loaf = entry as LoAFEntry; | ||
| const durationMs = Math.round(loaf.duration); | ||
| const blockingDurationMs = loaf.blockingDuration != null | ||
| ? Math.round(loaf.blockingDuration) | ||
| : null; | ||
|
|
||
| const relevantScripts = loaf.scripts?.filter((s) => s.sourceURL || s.sourceFunctionName); | ||
|
|
||
| const scripts = relevantScripts?.length | ||
| ? relevantScripts.reduce<Json>((acc, s, i) => { | ||
| acc[`script_${i}`] = serializeScript(s); | ||
|
|
||
| return acc; | ||
| }, {}) | ||
| : null; | ||
|
|
||
| const details = compact([ | ||
| ['kind', 'loaf'], | ||
| ['startTime', Math.round(loaf.startTime)], | ||
| ['durationMs', durationMs], | ||
| ['blockingDurationMs', blockingDurationMs], | ||
| ['renderStart', loaf.renderStart ? Math.round(loaf.renderStart) : null], | ||
| ['styleAndLayoutStart', loaf.styleAndLayoutStart ? Math.round(loaf.styleAndLayoutStart) : null], | ||
| ['firstUIEventTimestamp', loaf.firstUIEventTimestamp ? Math.round(loaf.firstUIEventTimestamp) : null], | ||
| ['scripts', scripts], | ||
| ]); | ||
|
|
||
| const blockingNote = blockingDurationMs != null | ||
| ? ` (blocking ${blockingDurationMs} ms)` | ||
| : ''; | ||
|
|
||
| const topScript = relevantScripts?.[0]; | ||
| const culprit = topScript?.sourceFunctionName | ||
| || topScript?.invoker | ||
| || topScript?.sourceURL | ||
| || ''; | ||
| const culpritNote = culprit ? ` — ${culprit}` : ''; | ||
|
|
||
| onEntry({ | ||
| title: `Long Animation Frame ${durationMs} ms${blockingNote}${culpritNote}`, | ||
| context: { details }, | ||
| }); | ||
| } | ||
| }).observe({ type: 'long-animation-frame', buffered: true }); |
There was a problem hiding this comment.
The PerformanceObserver instance created here is not stored or returned, making it impossible to disconnect or clean up the observer later. This could be a memory leak concern if the Hawk catcher is destroyed and recreated multiple times in the application lifecycle (e.g., in SPAs with dynamic module loading). Consider storing the observer instance and providing a cleanup mechanism, similar to how other addons handle resource management.
| /** | ||
| * Build a Json object from entries, dropping null / undefined / empty-string values | ||
| */ | ||
| function compact(entries: [string, JsonNode | null | undefined][]): Json { |
There was a problem hiding this comment.
utility function should be added to /utils and covered by tests
| interface LongTaskAttribution { | ||
| name: string; | ||
| entryType: string; | ||
| containerType?: string; | ||
| containerSrc?: string; | ||
| containerId?: string; | ||
| containerName?: string; | ||
| } |
There was a problem hiding this comment.
move types to the /types folder or the @hawk.so/types package.
and add clear jsdocs to each property.
| */ | ||
| function observeLongTasks(onEntry: (e: LongTaskEvent) => void): void { | ||
| if (!supportsEntryType('longtask')) { | ||
| log('Long Tasks API is not supported in this browser', 'info'); |
There was a problem hiding this comment.
Such logs should not be shown by default, only when user explicitly enables such logs
| const relevantScripts = loaf.scripts?.filter((s) => s.sourceURL || s.sourceFunctionName); | ||
|
|
||
| const scripts = relevantScripts?.length | ||
| ? relevantScripts.reduce<Json>((acc, s, i) => { |
| * | ||
| * @default enabled with default options (both longTasks and longAnimationFrames on) | ||
| */ | ||
| mainThreadBlocking?: false | MainThreadBlockingOptions; |
There was a problem hiding this comment.
why Long Animation Frames is configured via mainThreadBlocking option. It's confusing.
Maybe we can use a better naming
…ThreadBlocking for clarity
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Peter <specc.dev@gmail.com>
Co-authored-by: Peter <specc.dev@gmail.com>
…interfaces and improving script serialization
2 participants
Summary
send()pipeline with the blocking details in contextmainThreadBlockingsetting, or disabled entirely withfalseBrowser support