Skip to content
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

[Connector Builder] Add Segment event tracking #21686

Merged
merged 24 commits into from
Jan 25, 2023

Conversation

lmossman
Copy link
Contributor

What

Resolves #20863

Captures the events as listed in the Track (Connector Builder) tab of this spreadsheet: https://docs.google.com/spreadsheets/d/1lGLmLIhiSPt_-oaEf3CpK-IxXnCO0NRHurvmWldoA2w/edit#gid=1297906000
(except for the Connector Created event, since that functionality does not exist yet)

How

Uses the AnalyticsService to send various events to Segment. I followed the existing convention of not using i18n for the action descriptions as these are not displayed to users and only used for internal tracking.

In order to test this properly, you need to sign into Amplitude and search for events starting with Airbyte.UI.ConnectorBuilder.
Here is a screenshot with a bunch of these tracked events shown in that platform (the CSV export does not actually export these events, just the single user list, and I couldn't find a great way to export these as text)
Screen Shot 2023-01-20 at 5 50 23 PM

@lmossman lmossman requested a review from flash1293 January 21, 2023 01:51
@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 21, 2023
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

It seems like stream test success and failure are recorded, but the user action "Stream test" is not - I guess we should add that on button click.

// so the analytics events won't fire just from the user switching between streams, as desired
if (isFetchedAfterMount) {
if (errorMessage) {
analyticsService.track(Namespace.CONNECTOR_BUILDER, Action.STREAM_TEST_FAILURE, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is called on every state update:

  • Make an actual stream test which results in failure: works as expected
  • Afterwards, change something in the UI - another failure event is recorded (even if the user didn't click on test).

This happens because jsonManifest.streams changes its reference on every state update.

Idea to fix this: Use the streams via ref and re-run the hook on streamReadData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flash1293 since we separately decided to not send the stream JSON body in these events, I no longer had the issue of this depending on jsonManifest.streams and re-firing for every state change. However, I did need to add a couple more changes here to have this useEffect be dependent on dataUpdatedAt and errorUpdatedAt from the react-query, to make the events be tracked when a user clicks Test again and the results don't change. See commit: 09967f0

With these changes, I believe this is now working properly, but definitely lmk if you think anything else should be changed here.

@lmossman lmossman requested a review from flash1293 January 24, 2023 01:38
@lmossman
Copy link
Contributor Author

@flash1293 I believe I've addressed the feedback here and it should be ready for another look

@flash1293
Copy link
Contributor

Looks like the e2e test failure is legit - as the route is part of the workspace now, it shows the signup form:
Screenshot 2023-01-24 at 16 22 46

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, there is one thing that probably should be adjusted but I don't think another round of review is necessary - when switching the stream view by switching the stream name in the testing panel, no event is fired, but it's essentially the same thing as switching from the left hand side bar.

@@ -152,6 +149,10 @@ export const SchemaDiffView: React.FC<SchemaDiffViewProps> = ({ inferredSchema }
variant="secondary"
onClick={() => {
helpers.setValue(formattedSchema);
analyticsService.track(Namespace.CONNECTOR_BUILDER, Action.OVERWRITE_SCHEMA, {
actionDescription: "Declared schema ovewritten by detected schema",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit typo ovewritten -> overwritten (also in the other occurences)

@lmossman lmossman merged commit 60da9e4 into master Jan 25, 2023
@lmossman lmossman deleted the lmossman/connector-builder-event-tracking branch January 25, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connector builder: Add event tracking
3 participants