Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Set UTM etc when creating persons #214

Merged
merged 15 commits into from
Feb 26, 2021

Conversation

timgl
Copy link
Contributor

@timgl timgl commented Feb 25, 2021

Changes

This will automatically set UTM tags on the person initially and every time there's a new utm tag (ie first-touch and last-touch). It'll also set some other properties as initial (current url, browser etc).

Still need to test this but does this approach make sense?

The only downside of this is that if your first event for a user is a server event the initial properties won't get set. This might happen if your sign up form is on a marketing website that doesn't have posthog, which sends an event to a server that does. I think this is an unlikely enough scenario that this is worth it.

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@timgl timgl requested a review from mariusandra February 25, 2021 11:51
@jamesefhawkins
Copy link

The only downside of this is that if your first event for a user is a server event the initial properties won't get set. This might happen if your sign up form is on a marketing website that doesn't have posthog, which sends an event to a server that does. I think this is an unlikely enough scenario that this is worth it.

@yakkomajuri this is the sort of exception that would be really helpful in our docs / a tutorial - how to track where your users came from

@mariusandra
Copy link
Collaborator

Wouldn't it be easier to add into the $set_once property also all the initial $browser, $current_url, etc properties? That would solve the problem of "The only downside of this is that if your first event for a user is a server event the initial properties won't get set".

I assume that you opted out of that... and opted to add them as properties only when creating the person to save on DB traffic? Yet wouldn't we be able to handle that one extra Postgres query? ... and optimise in the future if necessary? If the initial props don't change, we won't add a new row into ClickHouse after all. Adding a filter for just $pageview events would also reduce potential Postgres queries here...

Thinking a step further, the idea was for the maxmind plugin to also set $country and $initial_country style properties on the user. I don't see any other way than using $set_once there. It will probably be quite a popular plugin, so we need to optimise this code/db path anyway.

Also, I tried re-implementing this as a plugin and ended up with this:

// UTM and Initial Properties Plugin
const campaignParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_content', 'utm_term']
const initialParams = ['$browser', '$browser_version', '$current_url', '$os', '$referring_domain', '$referrer']
const allParams = [...initialParams, ...campaignParams]

function processEvent(event, { config }) {
    if (event.event !== '$pageview') { 
        return event 
    }

    let setProperties = false
    const $set = {}
    const $setOnce = {}

    for (param of allParams) {
        if (typeof event.properties?.[param] !== 'undefined') {
            $setOnce[`\$initial_${param.replace('$', '')}`] = event.properties[param]
            setProperties = true
        }
    }

    for (param of campaignParams) {
        if (typeof event.properties?.[param] !== 'undefined') {
            $set[param] = event.properties[param]
            setProperties = true
        }
    }

    if (setProperties) {
        event.properties['$set'] = { ...event.properties['$set'], ...$set }
        event.properties['$set_once'] = { ...event.properties['$set_once'], ...$setOnce }
    }

    return event
}

Maybe it could even be just a plugin? Enabled by default for everyone? That would test the cloud system to its limits and require lazy VMs, but... maybe? :)

@timgl
Copy link
Contributor Author

timgl commented Feb 25, 2021

The problem isn't postgres queries, it's clickhouse queries. We'd insert a new person for each event, and merging people is super expensive on clickhouse.

@mariusandra
Copy link
Collaborator

I must be missing something then. We bail from creating anything in clickhouse if nothing changes in the props. If initial props have already been set, nothing will have changed.

@timgl
Copy link
Contributor Author

timgl commented Feb 25, 2021

Oh didn't realise this! In that case I think this makes sense.

In terms of plugins, I think this should be enabled for everyone and not sure why anyone would want to disable it, so it's a core bit of functionality. I don't want to clog the plugins list with something like this. Can we hide plugins and enable as default?

@mariusandra
Copy link
Collaborator

For sure, but then let's simplify this to just use $set_once?

@timgl
Copy link
Contributor Author

timgl commented Feb 25, 2021

Yep will do.

@timgl
Copy link
Contributor Author

timgl commented Feb 25, 2021

Done!

src/ingestion/utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I changed the type of properties and there's a flaky test that failed earlier. Once they're green I'll merge and release a new version.

@mariusandra mariusandra added the bump minor Bump minor version when this PR gets merged label Feb 26, 2021
@mariusandra mariusandra merged commit d2a1b21 into master Feb 26, 2021
@mariusandra mariusandra deleted the set-utm-and-other-initial-on-person branch February 26, 2021 10:35
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…ver#214)

* Set UTM etc when creating persons

* fix test

* Use slightly faster COUNT(*)

* Rename userShouldSetUTM to ensurePersonUpdateOnUtm for clarity

* Refactor ensurePersonUpdateOnUtm a bit

* Prettier

* only use $set and $set_once

* fix

* Add new prop

* remove new team prop

* use sets

* fix type

Co-authored-by: Michael Matloka <[email protected]>
Co-authored-by: Marius Andra <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants