Repository navigation
feat(instrumentation): add Long Animation Frames instrumentation - #429
SyedTashfin wants to merge 19 commits into
Conversation
d2aa73d to
501138f
Compare
There was a problem hiding this comment.
note: to be discussed in browser SIG and added in the registry
There was a problem hiding this comment.
I would not hold this PR on this. We can add these to the registry once it is ready.
There was a problem hiding this comment.
Acknowledged — the constants stay local to the instrumentation in semconv.ts until #404 lands in the registry. Nothing here depends on the registry shape, since the names are only used to build this instrumentation's log record, so it can move when #404 does, as @martinkuba noted above.
| } | ||
|
|
||
| private _isSupported(): boolean { | ||
| if ( |
There was a problem hiding this comment.
I've seen this in other instrumentations (more than two). IMO this is a candidate to be a utility function. out of the scope of this PR
There was a problem hiding this comment.
Agreed, and left out of this PR. Worth noting that the shared pieces this instrumentation does need now come from the base added in #278: assertPerformanceObserver for the support check and _safeExecute for containing a failing entry, so a general helper for the record-building loop would sit next to those rather than in this PR. Happy to open a follow-up issue for it if you want it tracked.
| }, | ||
| }; | ||
|
|
||
| const hook = this.getConfig().applyCustomLogRecordData; |
There was a problem hiding this comment.
If the apply function does get additional context is easier to use a LogRecordProcessor and do not more complexity to the instrumentation. ref: #359 (comment)
There was a problem hiding this comment.
I'd suggest to remove it. The entry contains all the context and is translated to a log. There is no user action attached or event related to it. This can be done with a LogRecordProcessor when configuring the LoggerProvider
|
@SyedTashfin The ticket is assigned to me because the instrumentation code is already completed. I think the cardinality of the code in your PR may be a bit high. I'll review in depth later. |
A LogRecordProcessor configured on the LoggerProvider covers the same case, and the emitted record already carries the full entry context. Addresses review feedback on open-telemetry#429.
|
Removed the Worth flagging: I've left the semconv attribute names local to the instrumentation for now and will move them to the registry once #404 is in. @overbalance you mentioned the ticket is yours because the instrumentation is already completed. I couldn't find a branch or a PR for it, so I can't tell what state it is in. If you're porting it, tell me and I'll close this one; I just don't want two implementations of the same thing running in parallel. If it's the size of the diff that's the concern, tell me which parts you'd cut and I'll strip it down. |
mquentin
left a comment
mquentin
left a comment
There was a problem hiding this comment.
Thanks for your contribution, I juste have a small suggestion and I would love to have your input on a question:
Suggestion:
I think having a small throttle safeguard for this instrumentation would be great but this instrumentation is indeed a good first step toward having a better performance monitoring.
My main question is
How could we aggregate these long animation frame ? I feel most of the value of these events comes from being able to identify which user interaction generated this long animation frame and on which page did it occurs.
Do you have any take on this ?
|
|
||
| const observer = new PerformanceObserver((list) => { | ||
| for (const entry of list.getEntries()) { | ||
| this._emitLongAnimationFrame( |
There was a problem hiding this comment.
Suggestion:
could we have a debounce or throttle mechanism here if somehow too many long animation framed occurred at the same time ?
I know there are supposed to have a meaningful duration for each frames and you're not supposed to have thousands of very small frames but just in case a throttle might be a good safeguard.
|
Answering the question first, then the throttle, because I disagree with that one. On aggregation: I think the record is already an attribution object, so this is mostly a grouping problem rather than a data problem. For which interaction — The other half of that join is already in this repo: the web-vitals instrumentation timestamps the INP record with For which page — nothing to add here. The SDK's Where I'd keep the line is not in the instrumentation. A rollup here needs a time window and a flush policy, which turns a faithful record emitter into a metric reporter with config surface. On the throttle: I'd rather not, and I think it would actively hurt. LoAF is a jank signal — entries only exist for frames past 50ms, and the browser caps the timeline buffer at 200 entries. A throttle drops the worst frames first: a two second jank that produces dozens of entries would report one, and the p95 downstream gets better while the page gets worse. That's the one failure mode I don't want from a performance instrumentation. If you want a hard ceiling, I'd do it with accounting instead: a per-callback cap, plus a dropped count on the next record so the loss is visible. Or reuse the shape |
@SyedTashfin Thanks for all the inputs and for answering my question. Indeed if the browser already caps the buffer at ~200 entries no need to implement something right now. I also feel that having a Suggestion:Given your meaningful inputs about the native browser ~200 entries cap and the existing out of the box timing interaction and vital aggregations, could you document this instrumentation to reflect these two main points ? |
| }); | ||
|
|
||
| try { | ||
| observer.observe({ |
There was a problem hiding this comment.
Can we only replay buffered frames on the first enable? Right now, I believe disabling and reenabling logs the same frames again.
There was a problem hiding this comment.
Done in 0ccb7f6, and it survived both merges of main: the observer asks for buffered only until the first successful observe(), so a disable() / enable() cycle resumes live observation instead of replaying what was already logged.
Two tests pin it in instrumentation.test.ts: replays buffered entries on the first enable only (the second observe() gets buffered: false) and keeps the buffered replay pending when the first observe fails (the flag is set only after observe() returns, so a browser that rejects the first call still gets its replay).
Unchanged by the base migration in 7c4eb4d.
Port the Long Animation Frames instrumentation from opentelemetry-js-contrib, emitting long-animation-frame performance entries as logs (matching the existing long-task pattern).
A LogRecordProcessor configured on the LoggerProvider covers the same case, and the emitted record already carries the full entry context. Addresses review feedback on open-telemetry#429.
`buffered: true` was requested on every `enable()`, so a disable/enable cycle replayed the frames the browser had retained and emitted the same frames again. Request the buffered replay on the first enable only, and keep it pending when the first `observe()` throws so a failed start does not consume it.
Cover the buffered replay and the browser's ~200 entry cap with the reason no client-side throttle is applied, how `first_ui_event_timestamp` joins a frame to the interaction that triggered it and to the web-vitals INP record, and the page-attribution caveat that comes with the replay.
13bee56 to
f610ec1
Compare
|
Both points are addressed. @JacksonWeber — fixed in @mquentin — documented in
It also records one caveat worth knowing up front: replayed frames carry the URL current at emit time, not the URL they happened on, via Also rebased onto Note on how this was written: an agent drafted the patch and the README section; I read the diff line by line and can answer for it in review. |
mquentin
left a comment
mquentin
left a comment
There was a problem hiding this comment.
LGTM in term of code and more than up to introduce this new experimental instrumentation.
Before my approval, I have a last suggestion:
Could add this new instrumentation in the demo sandbox: here
It would then be great to have a new btn that mock a long animation frame to generate the new event you introduced and demo the long animation frames instrumentation (on top have helping QAing the change of this PR by running the sandbox locally for other reviewers)
|
|
||
| --- | ||
|
|
||
| ### Long Animation Frames |
Register LongAnimationFrameInstrumentation in the sandbox SDK and add a Long Frame button that blocks the main thread for 300 ms. A long animation frame is reported only once a frame passes the 50 ms threshold, so the handler does real blocking work in the click task, which is what the browser attributes the frame to in `scripts[].invoker`. The button warns instead of doing nothing on browsers without the API.
Event-based instrumentations (navigation, resource timing, user action, long animation frames) emit a record with an eventName and no body, so the sandbox event log printed "[log] INFO · undefined" for all of them. Fall back to the event name when there is no body to render.
…-frame-instrumentation
|
Sandbox demo is in —
One fix was needed for the demo to be readable: event-based records carry an Branch is merged with Patch drafted with an agent; I read the diff line by line and can answer for it. |
| [ATTR_LONG_ANIMATION_FRAME_NAME]: entry.name, | ||
| [ATTR_LONG_ANIMATION_FRAME_ENTRY_TYPE]: entry.entryType, |
There was a problem hiding this comment.
name and entryType are always "long-animation-frame" per the spec, and for each script both are always "script" (lines 142-143). The event name already says what the record is, so I'd drop all four. They're repeated on every record and every script without telling the backend anything. The README row for entry_type says "normally" too, that can go with it.
|
|
||
| `first_ui_event_timestamp` is a `DOMHighResTimeStamp` on the same clock as `Event.timeStamp`, so a frame can be joined to the interaction that triggered it without any context plumbing. The `scripts` array then names the culprit: `invoker` (for example `DOMWindow.onclick`), `invoker_type` (`event-listener`), `source_url`, `source_function_name`, `source_char_position`, and the per-script split of `blocking_duration` against `forced_style_and_layout_duration`. | ||
|
|
||
| The other half of that join is already in this repository. The [Web Vitals](#web-vitals) instrumentation timestamps its INP record with `attribution.interactionTime` and puts the raw attribution in the record body when `includeRawAttribution` is enabled, so matching a frame's `first_ui_event_timestamp` against a `browser.web_vital` INP record yields the interaction target and type together with the scripts that blocked the frame: INP says which interaction was slow, Long Animation Frames say which script made it slow. |
There was a problem hiding this comment.
An exact match on first_ui_event_timestamp will miss frames. Per the spec it's the timeStamp of the first UI event whose listener ran in that frame, any UI event, while interactionTime is the start of the interaction's first event entry. Those only line up when that frame happened to handle the interaction's first event before anything else. A slow interaction can also overlap more than one frame (the one that delayed the input, the one running the handlers) and at most one of them can match.
web-vitals doesn't join on equality, it takes every LoAF that overlaps the interaction. With includeRawAttribution on, those frames are already in the INP record body as attribution.longAnimationFrameEntries, scripts included, so for INP the join is already done. Might be simpler to point at that and describe an overlap match for anything else.
…ng script fields The spec fixes the entry name to `long-animation-frame` and every script entry name to `script`, so `name` and `entry_type` were constant on every record and on every script entry. The event name already says what the record is, so drop all four. `invoker` and `sourceURL` are emitted as the browser reports them, which for an inline script or a listener defined on the page is the page URL including its query string. Add the `sanitizeUrl` config option that navigation, fetch and xhr already expose and route both fields through it, so a `token` in the page URL cannot leak through the scripts array while being stripped from `url.full`.
…P join notes The performance timeline buffer keeps the first ~200 entries and leaves out the newer ones once full, so it only bounds the buffered replay; a live observer still receives every frame. Drop the claim that it caps what the instrumentation emits, and describe the INP correlation as an overlap match rather than an exact `first_ui_event_timestamp`/`interactionTime` comparison, pointing at the frames web-vitals already inlines in the INP record body. Also document the `sanitizeUrl` option and drop the two attribute rows the implementation no longer emits.
…-frame-instrumentation
|
@adityareddy-dev all four are in The four fields — dropped.
One thing I didn't decide for you: should this instrumentation apply The buffer — you're right, I had it backwards. Fixed: the timeline buffer keeps the first ~200 entries and leaves the newer ones out once it's full, so it only bounds the buffered replay, a live observer still gets every frame, and it isn't a reason not to throttle. The sampling-hides-the-worst-frames point stands on its own. The INP join — rewritten as an overlap match. Merged |
|
Ran the long animation frame tests at On the default, I'd flip it, for the reason you gave, nobody goes looking for a URL inside It won't catch everything though. The semantic conventions name five query keys to redact by default ( Two more, mostly in the README. The other is page attribution, it isn't only the replayed frames. Every record goes out from the observer callback after the frame is over, so a frame that changed the URL gets stamped with the new one. I tried a click handler that blocks for 100 ms and then calls And one from the new text. The overlap match uses the frame's |
|
The sanitizer gaps are their own issue now, so they don't hold this one up. |
| start_time: script.startTime, | ||
| duration: script.duration, | ||
| execution_start: script.executionStart, | ||
| invoker: sanitize(script.invoker), |
There was a problem hiding this comment.
invoker isn’t always a URL. Event listeners can report DOMWindow.onclick, which throws in a sanitizer using new URL(url). That also skips the remaining frames in this callback because record construction is outside the error guard. Can we only sanitize URL-valued invokers and add a test with an event-listener entry and a URL-parsing sanitizer?
`invoker` is the invoking script's source URL only for `classic-script` and `module-script` entries; for an `event-listener` entry it is a selector-like string such as `DOMWindow.onclick`. `source_url` is a URL, but the browser reports an empty string when it cannot resolve one. Both were handed to `sanitizeUrl` unconditionally, so a sanitizer that parses its argument — `new URL(url)`, the obvious implementation — threw, and because the record was built outside the error guard, that throw skipped the remaining frames in the same observer callback. Only non-empty URLs now reach the sanitizer, and every entry is processed inside `safeExecuteInTheMiddle`, so a failure in one costs that frame alone. Also, from the same review: - `sanitizeUrl` defaults to `defaultSanitizeUrl`. The value sits inside the `scripts` array, where a page token is easy to miss, so it is on by default here rather than opt-in as `url.full` is for navigation, fetch and xhr. `sanitizeUrl: undefined` emits both fields unsanitized. - Add `browser.long_animation_frame.start_time`, so the overlap match in the README is expressible from the frame record. - Describe `blocking_duration` as the spec defines it — the sum over the frame's tasks longer than 50 ms, with the rendering time added to the longest — and drop the claim that scripts carry a split of it. - Page attribution applies to every record, not only the buffered replay: the observer callback runs after the frame, so a frame whose handler navigated is stamped with the new URL.
…-frame-instrumentation
|
Both points are fixed in @JacksonWeber — you were right on both counts, and the second one was worse than the first.
Tests: an @adityareddy-dev — the three from your last review are in the same commit:
Two things I'm not hiding:
Drafted with an agent from the review threads; I read the diff line by line and ran the checks locally, and I can answer for it in review. |
adityareddy-dev
left a comment
adityareddy-dev
left a comment
There was a problem hiding this comment.
Default sanitizing now leaves the selector invokers alone, and the buffer and INP notes cover what I raised.
One small README fix, start_time is relative to the time origin, while the record timestamp adds performance.timeOrigin. Could you drop 'the same value as the record timestamp' from that row?
`start_time` is the frame's start relative to the time origin, while the record timestamp adds `performance.timeOrigin` to it, so the row could not call them the same value. The page attribution note now states that relationship instead of implying the two are equal.
|
Dropped in Thanks for the review and the approval. |
…-frame-instrumentation
…-frame-instrumentation One conflict, in `sandbox/src/otel.ts`: main adds the fetch instrumentation and this branch adds the long animation frame one on the same import line. Both kept, both registered.
|
Merged current @david-luna — the three things you raised, as they stand now:
Local runs on this head: Could you take another look? If one of the three is still open in your view, tell me which and I will fix that one. |
There was a problem hiding this comment.
The start_time row now distinguishes the frame's relative time from the epoch record timestamp, which closes my last README note. I also checked the merge resolution, both the fetch and long animation frame imports and registrations are retained, and the long animation frame implementation is unchanged since my previous review.
|
@david-luna — adityareddy-dev approved this at Your three points from September are addressed as far as I can tell:
Your change request from 23 Sep is still standing, so the PR cannot merge. I tried to re-request your review and the org does not let contributors do that, so this comment is the ask. If something is still open for you, tell me which one and I'll fix that one — if not, could you clear the request? Separately, the workflow runs on this branch are still waiting for a maintainer to approve them, so the checks on the head are only EasyCLA; the local run on |
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-28 16:18 UTC Review the latest changes. Also blocked by: Merge conflicts. Status above doesn't look right?
|
…-frame-instrumentation
…nBase main replaced the @opentelemetry/instrumentation base with the package's own browser-native one (open-telemetry#278) and dropped the former from the package's dependencies, so this instrumentation imported a package it no longer declared. Move it to the new base. `_init()` asserts PerformanceObserver and the long-animation-frame entry type, throwing so the instrumentation stays off in browsers without the API, as the other observer-based instrumentations do. `enable()` and `disable()` become `_onEnable()` and `_onDisable()`, the `_isEnabled` flag becomes the base's `isEnabled()`, configuration is read from `_config`, and the per-entry and emit guards use `_safeExecute`. The buffered replay is unchanged: `buffered` is asked for only until the first successful `observe()`, so a disable/enable cycle resumes live observation without re-emitting frames that were already logged. The tests construct and enable explicitly, since the browser-native base has no constructor-time auto-enable, and read the sanitizer from `_config`.
|
Merged
Verification on
Review items: the three the dashboard lists are answered in their threads. The replay ask from @JacksonWeber was already implemented in Remaining blocker is not on my side: @david-luna's |
adityareddy-dev
left a comment
adityareddy-dev
left a comment
There was a problem hiding this comment.
Checked the move onto the new InstrumentationBase against resource-timing on main. _onEnable returns early when an observer exists, _onDisable disconnects it, and the isEnabled() check in the callback covers an entry that lands after disable. Buffered replay on the first enable only matches how resource-timing does it, so a re-enable doesn't emit old frames twice.
Which problem is this PR solving?
Browser instrumentations are being consolidated into this repository, and #401 asks for Long Animation Frames instrumentation. LoAF is the successor to the deprecated Long Tasks API.
This is a port of the implementation I first opened in opentelemetry-js-contrib#3751, adapted to the conventions here: it emits log records instead of the span-based version in contrib, matching the console and errors instrumentations.
Fixes #401
Short description of the changes
long-animation-frameperformance entries and emits each one as a log record carrying the frame timing breakdown (start_time,duration,blocking_duration,render_start,style_and_layout_start,first_ui_event_timestamp) and the per-script attribution (scripts).invokerforclassic-script/module-scriptentries, andsource_url) go throughsanitizeUrl, which defaults todefaultSanitizeUrl. That is on by default here unlikeurl.fullfor navigation, fetch and xhr: this value sits inside thescriptsarray where a page token is easy to miss. Values that are not URLs (DOMWindow.onclick, an unresolvedsource_url) are emitted as reported and never handed to the hook.enable()only, so adisable()/enable()cycle resumes live observation instead of re-emitting frames that were already logged.packages/instrumentation/src/long-animation-frame/(instrumentation, types, semantic attribute constants, tests), the./experimental/long-animation-frameexport and keyword inpackages/instrumentation/package.json, aLong Animation Framessection in the package README, and the sandbox demo with aLong Framebutton.InstrumentationBaseintroduced in refactor(instrumentation)!: add browser-native InstrumentationBase and migrate instrumentations #278.Type of change
How Has This Been Tested?
Local run on the pushed head, in
packages/instrumentation— 18 vitest cases insrc/long-animation-frame: buffered-entry emission and the full attribute set, the log timestamp taken from the entry start time, unsupported-environment guards, disable/re-enable, replay-on-first-enable-only, per-entry failure containment, and the sanitization contract (only non-empty URLs reach the hook, a sanitizer that parses its argument is safe to pass, one failing entry does not drop the frames after it).Whole tree on the same head (
7c4eb4d, node 26.12.1 / npm 11.20.0 asdevEnginesrequires):npx turbo build --forceuncached 5/5,npm run lint:ci(biome, tsc, eslint) clean,npm test(433 instrumentation tests in 21 files, 127 sdk tests in 15 files),npm run test:e2e29/29.The instrumentation extends the browser-native
InstrumentationBaseadded in #278, soenable()/disable()drive_onEnable()/_onDisable()andinit()asserts the API is present; the package no longer imports@opentelemetry/instrumentation.The workflow runs on this head are waiting for approval, so the runs above are the local stand-in for CI.
Generative AI disclosure
I used Hermes Agent to assist with repository research, porting the code to this repository's conventions, test drafting and documentation. I reviewed the diff line by line, ran the checks above on the pushed head, and take responsibility for the change.