-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃獰聽馃敡 Add Cross domain tracking for segment #16268
馃獰聽馃敡 Add Cross domain tracking for segment #16268
Conversation
@@ -97,6 +98,11 @@ export const AuthenticationProvider: React.FC = ({ children }) => { | |||
news: userData.news ?? false, | |||
}); | |||
|
|||
const previousAnonymousId = Cookies.get("ajs_anonymous_id"); | |||
if (previousAnonymousId) { |
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.
Check if the cookie exists and if so, set the anonymousId instead of creating a new one
user: jest.fn(), | ||
setAnonymousId: jest.fn(), | ||
init: jest.fn(), | ||
use: jest.fn(), | ||
addIntegration: jest.fn(), | ||
load: jest.fn(), | ||
trackLink: jest.fn(), | ||
trackForm: jest.fn(), | ||
ready: jest.fn(), | ||
debug: jest.fn(), | ||
on: jest.fn(), | ||
timeout: jest.fn(), |
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.
Needed because of new types
It seems that we are going to try moving cloud.airbyte.io to cloud.airbyte.com to avoid any cross-domain tracking issues. see: https://github.com/airbytehq/airbyte-cloud/issues/2623 for further details |
* master: (200 commits) 馃獰 馃Ч Display returned error messages on replication view (#16280) 馃帀 Source mixpanel: Use "Retry-After" header for backoff (#16770) 馃悰 Source google ads: mark custom query fields required (#15858) 馃獰 馃敡Remove useRouter hook (#16598) CDK: improve TypeTransformer to convert simple types to array of simple types (#16636) CDK: TypeTransformer - warning message more informative (#16695) Source MySQL: Add Python SAT to detect backwards breaking changes (#16445) remove eager (#16756) bump com.networknt:json-schema-validator to latest version (#16619) Remove Cloud from Kafka docs (#16753) Normalization Summaries table and read/write methods (#16655) comment out flaky test suite while it is being investigated (#16752) Update ConfigRepository to read protocol version (#16670) Use LOG4J2 to wrap connectors logs to JSON format (#15668) Update connector catalog (#16749) 馃獰 馃帹 Remove feedback modal from UI (#16548) Add missing env var for Kube overlays (#16747) Prepare for React v18 upgrade (#16694) 馃獰 馃悰 Fix direct job linking to work with pagination (#16517) Fix formatting (#16743) ...
const queryParams = new URLSearchParams(queryString); | ||
const ajs_anonymous_id = queryParams.get("ajs_anonymous_id"); | ||
if (ajs_anonymous_id) { | ||
Cookies.set("ajs_anonymous_id", ajs_anonymous_id); |
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'd suggest we save this in local storage instead, so we wouldn't need to add a new library here for cookies.
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 went with the option of storing it directly in cookies because I think it will be better to override Segment cookie instead of having two separate instances of ajs_anonymous_id
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 personally feel at that point we'd be "reusing" and overwriting something that is internal to how Segment works, and I feel just using our own local storage flag and just communicate with Segment via their API (which we anyway still do), feels like the cleaner approach of not running into any issues (e.g. what if the segment library actually decides overwriting the ajs_anonymous_id
cookie again in some scenarios with their own value, and we lost our original one).
That said, given that we're planning to remove this code in a couple of weeks again once we moved to cloud.airbyte.com I'm fine leaving this however you prefer :)
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.
Need to withdraw my comment :D I tested this, it actually is exactly what happens for me, the ajs_anonymous_id
cookie is set from the query but then shortly afterwards rewritten (by segment) to a random UUID again, and thus we'd lose the parameter. So I think we'll need to store it in localsStorage.
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.
Peek.2022-09-21.15-38.mp4
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 will double check this, because I'm not seeing Segment overriding it:
https://www.loom.com/share/e305ec6d955b49fe9e87cd6d21008b89
Trying your localStorage proposal was not working as Segment was indeed creating a new ajs_anonymous_id
I'll keep working on this
39431d9
to
6add02b
Compare
const ajs_anonymous_id = queryParams.get("ajs_anonymous_id"); | ||
|
||
if (ajs_anonymous_id) { | ||
localStorage.setItem("ajs_anonymous_id", JSON.stringify(ajs_anonymous_id)); |
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.
Saving in localStorage
per Tim's suggestion. Also better practice :) Thanks!
@@ -150,6 +156,7 @@ export const Routing: React.FC = () => { | |||
: null, | |||
[user] | |||
); | |||
useAnalyticsSetAnonymousIdFromLocalStorage(); |
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.
Extract anonymousId logic into its own hook
|
||
useEffect(() => { | ||
if (anonymousId) { | ||
analyticsService.setAnonymousId(JSON.parse(anonymousId)); |
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.
Here is the key method, provided by Segment to manually override an anonymousId
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.
@timroes I simplified the approach and followed your suggestion about |
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.
Tested locally and it seems to work and setting the correct id in Segment. Some minor code comments left. Given we are about to remove this code hopefully soon again, I consider them all optional.
if (anonymousId) { | ||
analyticsService.setAnonymousId(JSON.parse(anonymousId)); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
We'd usually just add the analyticsService
here as well. It's a stable reference so it won't cause us problems with rerendering, but helps not needing to disable eslint rules here.
const ajs_anonymous_id = queryParams.get("ajs_anonymous_id"); | ||
|
||
if (ajs_anonymous_id) { | ||
localStorage.setItem("ajs_anonymous_id", JSON.stringify(ajs_anonymous_id)); |
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.
Given that we only want to store a string here, I think we should get rid of the JSON.stringify
(and JSON.parse
above), which just add unnecessary performance overhead and making the types less safe (since JSON.parse
does not know what the result type will be).
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.
It wasn't working correctly without stringify
+ parse
@@ -76,6 +76,17 @@ export const useAnalyticsIdentifyUser = (userId?: string, traits?: Record<string | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
}, [userId]); | |||
}; | |||
export const useAnalyticsSetAnonymousIdFromLocalStorage = (): void => { | |||
const analyticsService = useAnalyticsService(); | |||
const anonymousId = localStorage.getItem("ajs_anonymous_id"); |
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'd suggest we move this into a getSegmentAnonymousId
method into the crossDomainUtils
file also, so we can keep all the knowledge about how and where it's stored local within this file.
* Add Cross domain tracking for segment * add email in identify call to test with Hubspot * save cookie on local storage, create hook to override Segment cookie * PR comments
* Add Cross domain tracking for segment * add email in identify call to test with Hubspot * save cookie on local storage, create hook to override Segment cookie * PR comments
What
This PR will allow us to pass
ajs_anonymous_id
Segment cookie from users coming fromairbyte.com
intocloud.airbyte.io
This PR also add @types/segment-analytics instead of having to define our own types.
Demo: https://www.loom.com/share/bb0d5ed9a4c34121b01db6957c60dd23
Updated approach: https://www.loom.com/share/40eae27720f34cbc8dc3956868b6e2c3
How
We will receive
ajs_anonymous_id
as a query param and save it in a cookie (this way even if the user does not sign up in that moment, we'll still have it)We add
ajs_anonymous_id
whenever it exist to the firstanalytics.track()
after a user is successfully created in cloud.馃毃 User Impact 馃毃
No impact.