-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 new source / destination test connection success / failure tracking events #12526
Conversation
…nection is being tested and remove from creation
Update new destination actions with useTrackAction hook
action: "Select a connector", | ||
connector_destination_definition: connector?.name, | ||
trackNewDestinationAction("Select a connector", { | ||
connector_destination: connector?.name, |
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.
The PM's spec for these events calls this property connector_destination
. Not all destination events are using this (yet) but at least it makes these actions consistent.
connector_source_definition: connector?.name, | ||
connector_source_definition_id: sourceDefinitionId, | ||
trackNewSourceAction("Select a connector", { | ||
connector_source: connector?.name, |
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.
The PM's spec for these events calls this property connector_source
. Not all source events are using this (yet) but at least it makes these actions consistent.
connector_source_definition_id: sourceDefinitionId, | ||
trackNewSourceAction("Select a connector", { | ||
connector_source: connector?.name, | ||
connector_source_id: sourceDefinitionId, |
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.
The PM's spec for these events calls this property connector_source_definition_id
. However, it's evenly split with both names across the codebase. For now, the name stays this way, but I will create an issue to make this consistent.
… new source actions
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.
Unsure how to test analytics calls locally, code LGTM.
…ng events (#12526) * Consolidate track new action analytics calls into hook * Move new source and destination test tracking to where the actual connection is being tested and remove from creation * Use consistent properties across all track new source actions * Make track action hook more generic and support new destination actions Update new destination actions with useTrackAction hook * Use connector_source_definition_id over connector_source_id for track new source actions
What
Currently, the source and destination connection test was not sending the events on test connection failure. Additionally, some of the properties passed down were inconsistent. This fixes both of those issues.
How
This change does a couple of things:
It moves the test connection tracking events to where the connection is being tested
Prior to this change, the connection was tracking the test connection success or failure events after creating the source or destination. However, if the test connection fails, it would never call create connection and the failure events would not be triggered.
Now all the test connection events happen when the test connection event is called instead.
It creates a hook for tracking that respects the tracking spec
Adds a new hook
useTrackAction
which respects the spec set for tracking. For this change, the hook is only used for the new source / new destination action events but could be expanded beyond the current use as separate issues. The hook ensures that the correct event name is set and the correct event properties are used.Recommended reading order
Start from the bottom and work your way to the top