Skip to content

Conversation

kelsonpw
Copy link
Contributor

@kelsonpw kelsonpw commented Sep 16, 2020

Description

This PR introduces a refactor to _trackParamsAndReferrer. It seperates the collecting of raw attribution values (utm, gclid, referrer) into pure functions, and then adds a new function _collectParamsAndReferrer that is almost identical to _trackParamsAndReferrer but just avoids making an additional identify call. credit to @kelvin-lu for helping think of this.

Here is how I would use this in the product/corpsite:

    amplitude.getInstance().onInit((client) => {
      const isNewSession = client.isNewSession();
      if (isNewSession) {
        const attributionProperties = client._collectParamsAndReferrer();
        client.logEvent('attribution captured', attributionProperties);
      }
    });

This is related to a recent Growth Eng effort to audit and optimize our UTM tracking and attribution. We would like a way to log an Amplitude event whenever attribution data is captured (utm, referrer, gclid). This is to compare our Amplitude SDK attribution with our alternative UTMz tracking setup on the corpsite & blog (already added a similar event here: https://github.com/amplitude/amplitude.com/pull/133). The SDK does not need to be the one logging the event, we just need access to the data.

Jira Ticket

image

Why we need this

We want to be able to get a full picture of every UTM/attribution state that a user has had during their Journey with Amplitude. Because Identify calls are not queryable, we are unable to devise a query that returns the information we are looking for. Also, because attribution consists of many properties (utm_source, utm_medium, utm_campaign, etc.), not all of them will have values. We do not require the SDK to send the event, we just need access to the data.

Updated Context:

I met with @kelvin-lu last week to discuss this PR. He suggested that it might be possibly to query using pre-existing events such as pageview. I spent sometime investigating this approach and determined that it is not viable. Even though pageview events have the attribution state present in user properties, they do not signal that attribution was actually captured at that exact point (which is what we are looking for). Another idea I discussed with Kelvin was the ability to query identify events, but after speaking with Tanner, this approach has been explored and is impossible.

New solution

I proposed a solution in PR #295, but it is not ideal. Until we fully determine that other users might like the auto attribution logging behavior, I believe this solution is a better alternative in the meantime. cc @haoliu-amp

@kelsonpw
Copy link
Contributor Author

kelsonpw commented Sep 16, 2020

@haoliu-amp and @kelvin-lu I ended up closing out my other PR in favor of this one. This new approach avoids adding actual Amplitude events to the SDK while still satisfying the needs of Growth eng. We just need access to the captured UTM's and then we can send the event ourselves in each project.

Tanner did mention that it is possible other customers might want attribution logging, but we haven't received any requests so I feel like this is a safer bet for the time being (also considering we want to get these events in place in a timely manner). Please let me know if you have any questions at all.

Copy link
Contributor

@kelvin-lu kelvin-lu left a comment

Choose a reason for hiding this comment

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

There might be some confusion as to why _collectParamsAndReferrer is not being called by _trackParamsAndReferrer, but I think the control is otherwise solid. Some considerations and a request for the test page :)

*/

AmplitudeClient.prototype._collectParamsAndReferrer = function _collectParamsAndReferrer() {
const utmProperties = this._getUtmProperties();
Copy link
Contributor

Choose a reason for hiding this comment

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

you should consider only collecting these if the option to get them is turned on - to mirror _trackParamsAndReferrer

Copy link
Contributor

Choose a reason for hiding this comment

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

naming nit: _collectParamsAndReferrer -> _getParamsAndReferrer or all of the below get to collect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great call on both. I changed the naming to _getParamsAndReferrer and I updated it to only fetch each piece of data if the functionality has been enabled in options

@kelsonpw kelsonpw requested a review from kelvin-lu September 16, 2020 23:51
@kelsonpw
Copy link
Contributor Author

There might be some confusion as to why _collectParamsAndReferrer is not being called by _trackParamsAndReferrer, but I think the control is otherwise solid. Some considerations and a request for the test page :)

@Kelvin good point about it being confusing not using getParamsAndReferrer inside of trackParamsAndReferrer. I ended up refactoring again to correct this. No behavior has changed, just moved. One weird thing is that now _saveGclid _saveReferrer and _initUtmData literally do the exact same thing (make a single call to _sendParamsReferrerUserProperties). Do you think its OK as is, or is it worth considering removing those three functions and just calling _sendParamsReferrerUserProperties in their place?

Copy link
Contributor

@kelvin-lu kelvin-lu left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for working through this! :)

@kelsonpw kelsonpw closed this Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants