PostHog / posthog

🦔 PostHog provides open-source product analytics, session recording, feature flagging and A/B testing that you can self-host.
https://posthog.com
Other
21.63k stars 1.29k forks source link

Enable session recordings from posthog.init #13367

Open lharries opened 1 year ago

lharries commented 1 year ago

Is your feature request related to a problem?

Currently, you need to turn on session recordings from the UI. It would be cleaner to be able to control it from the config and appeals to our highly technical ICP.

Describe the solution you'd like

The cleanest solution would likely be turning on session recordings if either session_recording is true | object. Similar to autocapture. You can then turn it off explicitly by setting it to false like so:

posthog.init('...', { session_recording: true }) // session recordings is now on
posthog.init('...', { session_recording: { blockClass: 'no-capture', } }) // session recordings is now on and uses the block class no-capture

posthog.init('...', {}) // session recordings is on only if it's turned on in the UI

posthog.init('...', { session_recording: false}) // session recordings is explicitly off

However, this would likely accidentally turn on session recordings for the small number of users that have session recordings turned off in the UI but have an object such as { blockClass: 'no-capture' } set.

The current options for session recording are:

posthog.init('...', {
  disable_session_recording: true, // turns it off even if it's on in the UI
  session_recording: {} // config block
})

Having enable_session_recording as well as disable_session_recording seems very messy.

Some potential options:

  1. Have session_recording: true | object => turn on session recordings and let people know that this change will be happening and do a major version bump i.e. 1.36 => 2.0 (this is what I'd recommend). Could instead do a minor version bump but a bit more risky
  2. Move from session_recording to session_recordings (with an s), keeping backwards compatibility with session_recording but enabling you to set session_recordings: true | object => to turn on session recordings
  3. Add enable_session_recording as well as disable_session_recording. Although I think this will end up being quite confusing and not as clean?

I'm happy to implement this (or someone else can) once we've decided the way forward

Describe alternatives you've considered

Maintain the need to turn on session recordings only via the UI

Thank you for your feature request – we love each and every one!

lharries commented 1 year ago

cc @benjackwhite @mariusandra @annikaschmid

benjackwhite commented 1 year ago

The overall idea here seems reasonable to me. Not sure about Option 2 (renaming to session_recordings). In the context of a client it is singular. We mostly use the singular form elsewhere as well. I would perhaps be tempted to break that out to a different issue where we solidify when to use plural and when to use singular, rather than including it here.

Also, I'm not sure about changing the config type. Introducing a breaking change if we can avoid it seems unnecessary. We anyway need to support Point 3 in some form as users may want to configure session_recording with recordings forcefully disabled and then manually enable them later.

I would propose keeping the object and adding a property that clearly describes how this works. Not super sold on overrideEnabled but something like it:

// Recordings on always
posthog.init('...', { session_recording: { overrideEnabled: true } })
posthog.init('...', { session_recording: { overrideEnabled: true, blockClass: 'no-capture' } })

// Recordings off always
posthog.init('...', { session_recording: { overrideEnabled: false } })
// backwards compatibility - no longer documented as a property 
posthog.init('...', { disable_session_recording: true , session_recording: { } }) 
// backwards compatibility but new property takes precedence
posthog.init('...', { disable_session_recording: false , session_recording: { overrideEnabled: false } }) 

// Recordings on only if the server side flag is on
posthog.init('...', {})
posthog.init('...', { session_recording: { blockClass: 'no-capture' }) // session recordings is explicitly off

We deprecate the disable_session_recording property, mapping it for backwards compat:


const clientForceEnabled = ph.session_recording?.enabled ?? ph.disable_session_recording ?? undefined

if (typeof clientForceEnabled === "boolean") {
  actuallyEnabled = clientForceEnabled
} else {
  actuallyEnabled = await serverEnabled()
}
mariusandra commented 1 year ago

I worked in the posthog-js codebase today and almost renamed register_once to registerOnce, when adding registerSession.

The JS-way is to use camelCase when naming your variables. For whatever reason, the library we based ours on used underscore_case.

A lot of the new code uses camelCase, and the entire library is a large pot of snake and camel stew.

If we want to nail slickness, sleekness and slick mode, we should make sure our integrations feel at home in each language, and that means deprecating all underscore_case methods and config options (they'll show up in the IDE as crossed out but continue working). This way we can add a sessionRecording option that works the way we'd like it to, without worrying about the old session_recording option.

lharries commented 1 year ago

@benjackwhite overall makes sense but I think overrideEnabled is too verbose if we want this to be the default way people turn on session recordings rather than feature flags. How about this instead:

posthog.init('...', { session_recording: { enabled: true } })

and then the button to turn it on only applies when the toggle hasn't been passed in?

I wonder if renaming all to camelCase might not be worth the effort for now? But don't feel super strongly about this

benjackwhite commented 1 year ago

Why do we want this to be the default way for people to turn it on? Ideally we don't want people to ever do this in the client and always do it via the server side controls.

I don't mind enabled but my concern here is that people will read that and think "oh I have to set that to enable recordings" which is not what we want people to do. Hence having a more explicit term.

I think I'm lost on the premise that we want this to be the way people configure Recordings. Maybe you can elaborate why that is the case?

lharries commented 1 year ago

Main reason would be almost everything else is configured from the code and feels more inline with being a devtool aimed at engineers.

One ideas was during onboarding it'd be interesting if as you toggle stuff on/off it changes the code config.

But I'm keen to keep 1 click turn on via the UI too

Up to you and your team to decide on this and the name/format/if you want to implement this