-
Notifications
You must be signed in to change notification settings - Fork 142
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
feat: canvas recording support #946
Conversation
Size Change: +2.39 kB (0%) Total Size: 760 kB
ℹ️ View Unchanged
|
@@ -491,6 +507,12 @@ export class SessionRecording { | |||
} | |||
} | |||
|
|||
if (!_isNull(this._recordCanvas) && !_isNull(this._canvasFps) && !_isNull(this._canvasQuality)) { |
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.
can we have a default fallback for _canvasFps
and _canvasQuality
instead of not enabling canvas at all?
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.
I considered that but didn't want to choose anything arbitrary before we knew what a sensible value might be. In reality they should never be null because we'll set the default on the backend so this is really more of a typing constraint
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.
the downside is that canvas recording can only start after the decide API has responded but by definition, recording should start recording until the decided responds, and only continue recording if it's enabled otherwise discard the recordings, so I guess we need to have some default values.
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.
IIRC what I meant above is related to cost control, if we don't need this right now, happy to merge as it is, approving it anyway.
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.
yep, better to start default disabled now because we can switch to default enabled easier than the reverse IMO
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.
Its not about the _recordCanvas
but rather the _canvasFps
nd _canvasQuality
.
before the decide response has been returned the client buffers all recording data
Without default (hardcoded) config for _canvasFps and _canvasQuality, this won't be possible since the 2 properties only have value after decide has responded.
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
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.
LGTM... won't merge till you're back since it's in your 🧠
Changes
Related to PostHog/posthog#14555
Adds the necessary config to control canvas recording of sessions remotely. While we experiment before launch we have decided to make this a remote config so that we have better control.
It sets an FPS and quality value for capturing canvases as images based on the decide response. Should nothing be returned or partial config returned the SDK will not enable canvas recording for the session.
Checklist