-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: include measurement Id as event param #57
fix: include measurement Id as event param #57
Conversation
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.
looking good. a few suggestions
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.
Great start thus far. Besides Rob's comments, I have some ideas on how we can clean up some of the code you're moving around.
this.sendCommerceEventToGA4( | ||
mapGA4EcommerceEventName(event), | ||
ga4CommerceEventParameters | ||
); | ||
return true; |
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 took a look at mapGA4EcommerceEventName
and it looks like it's possible that null
may be returned in place of an eventName
. While it may be an edge case, it's something we should consider handling.
Maybe we can event validate that the response is actually a non-empty string as well.
Probably out of scope for your change but I think it's a good defensive practice so we don't accidentally send an undefined
event to gtag.
this.sendCommerceEventToGA4( | |
mapGA4EcommerceEventName(event), | |
ga4CommerceEventParameters | |
); | |
return true; | |
var eventName = mapGA4EcommerceEventName(event); | |
if (typeof eventName === 'string' && eventName.length > 0) { | |
this.sendCommerceEventToGA4( | |
mapGA4EcommerceEventName(event), | |
ga4CommerceEventParameters | |
); | |
} else { | |
return false; | |
} | |
return true; |
return false; | ||
} | ||
|
||
this.sendCommerceEventToGA4( |
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.
Same comment here about checking if eventName
is a non-empty string.
event.ProductAction.TotalAmount || | ||
null; | ||
|
||
this.sendCommerceEventToGA4( |
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.
Same comment about empty strings
var customFlags = event.CustomFlags, | ||
GA4CommerceEventType = customFlags[GA4_COMMERCE_EVENT_TYPE], | ||
ga4CommerceEventParameters; |
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 think we should start splitting up our variable declarations when we migrate code.
var customFlags = event.CustomFlags, | |
GA4CommerceEventType = customFlags[GA4_COMMERCE_EVENT_TYPE], | |
ga4CommerceEventParameters; | |
var customFlags = event.CustomFlags; | |
var GA4CommerceEventType = customFlags[GA4_COMMERCE_EVENT_TYPE]; | |
var ga4CommerceEventParameters; |
Co-authored-by: Alex S <[email protected]>
## [1.4.1](v1.4.0...v1.4.1) (2024-03-11) ### Bug Fixes * Include send_to as event param to support multiple measurement ids ([#57](#57)) ([7d8696d](7d8696d))
Summary
A customer with more than one GA4 kit connection reported an issue with events being forwarded to the other GA4 connections even though they had forwarding rules set in the UI. After further investigation, it seemed that the issue was due to us using the global gtag('event', '<event_name>', {<event_params>}); per google API docs here. However, using this method will result in forwarding every event to every available Google instance with a measurement ID, wether it has been initialized by our integration or even outside of mParticle. Google does allow to include a measurement Id as part of the {<event_params>} to forward only to the specific GA4 instance associated with a specific measurement ID (the one that has been set by the customer in the UI in connection settings) as mentioned here in their recommended parameters docs. So the fix was to include the measurement ID as part of the params in all of our events that are forwarded to GA4 which fixes this issue and also fixes another issue where an event would be sent multiple times to the GA4 instance per one mParticle event if the customer had multiple GA4 kit connections setup
Testing Plan
E2E testing done with local overrides on the following cases and were all successful and resolved the issue:
Master Issue
Closes https://go.mparticle.com/work/REPLACE