-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 🔧 Add Segment track event to suggested destinations experiment #17328
🪟 🔧 Add Segment track event to suggested destinations experiment #17328
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.
One code comment. Once that is solved, LGTM. Have not tested locally.
@@ -65,6 +68,7 @@ export const DestinationForm: React.FC<DestinationFormProps> = ({ | |||
actionDescription: "Destination connector type selected", | |||
connector_destination: connector?.name, | |||
connector_destination_definition_id: destinationDefinitionId, | |||
...(trackParams && { ...trackParams }), |
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.
...(trackParams && { ...trackParams }), | |
...trackParams, |
You can spread undefined
into an object, it will not add anything in that case, thus no need to check if the object existed here.
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 test failure looks actually like a legit fallout form this change, and need to be addressed still.
…ations * master: (75 commits) source-sentry: migrate to per-stream states (#17466) Greg/clickhouse polishing (#17483) upgrade debezium version to 1.9.6 (#17459) 🐛 Source Twilio: Lookback_window config (#17478) hide S3 source connector from catalog (#17472) 🪟 Migrate styles for Connection-related Components (#17339) Added new title (#17480) Refactor & convert `PageTitle` to SCSS (#17139) updated releaseStage for zendesk-talk (#17477) [low-code] Apply log level to stream loggers (#17284) 🐛 Source Salesforce: filter out objects not supported by the Bulk API (#17453) Source Marketo: certify GA (#17445) Update greenhouse paginator (#17429) Add some services start validation to acceptance_tests.sh (#17425) 📖 Removes $ from terminal commands to allow direct copying. (#17467) migrate source GA connectors to per-stream states (2) (#17410) Source Klaviyo: bump CDK dependency (#17422) Source Pinterest: change releaseStage to GA (#17045) Source Pinterest: Set start_date dynamically based on API restrictions for lookup (#17387) updated releaseStage to generally_available (#17374) ...
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.
Code LGTM
…rbytehq#17328) * Add segment track event to suggested destinations experiment Closes airbytehq#17257 * Fix spec
Closes #17257
What
We need to measure how the suggested destinations experiment (#16355) performs and we need different params in the track event to be able to do so.
How
connector_destination_suggested
and override default tracking params if the action comes from a featured destination.🚨 User Impact 🚨
No impact