⚗️ Add canvas change detection for Session Replay - #4949
⚗️ Add canvas change detection for Session Replay#4949BeltranBulbarellaDD wants to merge 20 commits into
Conversation
Bundles Sizes Evolution
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: e9e087d | Docs | View more details | Give us feedback! |
BeltranBulbarellaDD
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 685c28e8ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1e132ae to
1bc1fbf
Compare
434f477 to
ad260aa
Compare
3ec079f to
3a2a2d7
Compare
ad260aa to
07a7e6c
Compare
3a78417 to
dc6b3d5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc6b3d5c72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b056d28805
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b97a735017
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| export function createCanvasManager(): CanvasManager { | ||
| const dirtyCanvases = new Set<HTMLCanvasElement>() |
There was a problem hiding this comment.
issue: When an enabled recording observes canvas churn (for example, charts that create, draw, and remove canvases), this Set keeps every removed dirty canvas and its backing bitmap reachable. Removal mutations never delete canvases, and no production code in this commit calls getDirtyCanvases() or clearDirtyCanvases(), so the pruning inside the getter never runs and memory grows for the lifetime of the recording; detached canvases need weak bookkeeping or cleanup when they are removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is accurate, though the headline ("avoid strongly retaining...") points in the wrong direction, I think. You need to iterate the contents of this set, it seems, so strongly retaining is necessary. However, you should have a way to remove canvases from the set of dirty canvas, and you should perform this removal in trackMutation.ts when canvas elements are removed from the DOM.
There was a problem hiding this comment.
Yeah I think I get what you mean but I think that's scope for the next PR. Something like this?
| } | ||
|
|
||
| export function createCanvasManager(): CanvasManager { | ||
| const dirtyCanvases = new Set<HTMLCanvasElement>() |
There was a problem hiding this comment.
This is accurate, though the headline ("avoid strongly retaining...") points in the wrong direction, I think. You need to iterate the contents of this set, it seems, so strongly retaining is necessary. However, you should have a way to remove canvases from the set of dirty canvas, and you should perform this removal in trackMutation.ts when canvas elements are removed from the DOM.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe5e50cbc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isCanvasElement(element)) { | ||
| transaction.scope.canvasManager.markCanvasDirty(element) | ||
| } |
There was a problem hiding this comment.
issue: When sessionReplayCanvasRecording is absent, disabled, or configured with a zero frame rate, serialization still unconditionally adds every canvas to the manager's strongly held Set. Fresh evidence in the final diff is that the manager is now always created, while the canvas tracker is the only component gated by configuration; because no production path calls getDirtyCanvases() or clearDirtyCanvases(), applications that repeatedly create and remove canvases retain those elements and their backing bitmaps for the recording's lifetime even though canvas capture is off. Gate these serialization and mutation updates on the enabled configuration.
Useful? React with 👍 / 👎.
| if (typeof CanvasRenderingContext2D !== 'undefined') { | ||
| CANVAS_2D_DRAWING_METHODS.forEach((method) => { | ||
| instrumentationStoppers.push( | ||
| instrumentMethod(CanvasRenderingContext2D.prototype, method, ({ target: context, onPostCall }) => { | ||
| onPostCall(() => markCanvasDirty(context.canvas)) |
There was a problem hiding this comment.
issue: When a browser loses and later restores a 2D canvas context after that canvas has been marked clean, restoration resets the context state and backing bitmap without calling any of these instrumented drawing methods or changing a size attribute. The manager therefore continues reporting the canvas as clean, so replay can retain the pre-loss frame indefinitely unless the application subsequently performs another tracked draw; listen for the canvas contextlost/contextrestored events and mark the affected canvas dirty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feca8f2c1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isCanvasElement(node)) { | ||
| transaction.scope.canvasManager.markCanvasClean(node) | ||
| } |
There was a problem hiding this comment.
issue: When a canvas is appended, drawn into, and removed before the mutation batch is processed, drawing adds it to dirtyCanvases, but the node has no serialized ID, so processRemovedNodes() returns at the preceding nodeId === undefined check and never reaches this cleanup. Fresh evidence after the earlier retention report is that the new removal cleanup still excludes exactly these unserialized nodes; because no production path currently calls getDirtyCanvases() to prune them, repeated transient canvases and their backing bitmaps remain retained for the recording lifetime.
Useful? React with 👍 / 👎.
| emitStats: EmitStatsCallback, | ||
| scope: RecordingScope | ||
| ): void { | ||
| markCanvasDirtyFromMutationRecords(mutations, scope.canvasManager) |
There was a problem hiding this comment.
We want a deeper integration than this. What I'd do is to remove markCanvasDirtyFromMutationRecords totally, and instead handle the work you need to perform in processAttributeMutations(). (Probably around the point in the loop where you see if (attributeName === 'value').)
Also, given that we currently don't capture any information about attribute namespaces in the recording, I'd recommend simply dropping those changes in this PR. The recorder generally does not work correctly with XHTML documents today, and I don't think we should try to solve that problem as part of these changes; it's a separate issue. I'm telling you this as part of this comment so that you don't worry about the fact that you don't have access to the XML namespace of the mutated attributes in processAttributeMutations() -- that's OK for now.
| const nodeId = nodeIds.get(node) | ||
| if (nodeId === undefined) { | ||
| continue // This node wasn't serialized. | ||
| } |
There was a problem hiding this comment.
Please don't remove these lines; we should never be tracking any information about nodes that don't have node ids. You probably felt the need to do this because in trackCanvas.ts you are unconditionally adding canvases to the dirty set when they are mutated; the right approach is to only do that if the canvas element has a node id already, and to additional mark canvases as unconditionally dirty when they are first serialized (i.e., when they are first assigned a node id).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9e087d596
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Motivation
Session Replay canvas capture can be expensive. Before capturing a canvas, we need to know whether its bitmap or relevant DOM state changed so unchanged canvases can be skipped and bandwidth can be reduced.
Depends on #4947.
Design references:
Changes
WeakSet-backed canvas manager that treats new canvases as dirty and tracks canvases that have already been marked clean without retaining detached elements.fillRect,drawImage,fill,stroke,putImageData,clearRect, and related methods.MutationObserverto detect canvaswidth/heightchanges across property and attribute mutation paths, including reinsertion and inserted subtrees.attributeNamespaceso namespaced attributes such asx:widthare not mistaken for the real canvaswidthcontent attribute.Scope
This PR adds the dirty-canvas prefilter and change detection. It does not yet implement periodic
toBlob()capture, image downscaling, hash comparison, or sending canvas images. Those pieces can use the manager to capture only canvases currently considered dirty and mark them clean after processing.The E2E scenario uses a test-only probe of the recorder's internal
WeakSetbecause canvas image capture and request assertions are not part of this change yet.Validation
test/e2e/scenario/recorder/canvas.scenario.ts; it was not run in this validation pass.