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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃獰 馃敡 Add proper page view events for Segment #16220

Merged
merged 4 commits into from
Sep 5, 2022

Conversation

letiescanciano
Copy link
Contributor

What

Closes #15849
There was a bug preventing analytics.page being fired on page views.

How

This PR changes a the old implementation where we were checking the url to determine what event should we fired (even though it wasn't working because a bug in how those routes were being checked) to calling useTrackPage() directly in every page component.

Main reasons of this change:

  • We avoid having to add a new check in that old getPageName method, avoiding also regex and string checks.
  • We have all of our tracked routes/event names in one place pageTrackingCodes
  • When removing a page, we also removed it's page view event.
  • When adding a page, we should make sure to ALWAYS include useTrackPage() and create the needed code.

馃毃 User Impact 馃毃

No user impact, just better analytics

@letiescanciano letiescanciano added area/frontend Related to the Airbyte webapp growth-eng labels Sep 1, 2022
@letiescanciano letiescanciano requested a review from a team as a code owner September 1, 2022 14:01
@github-actions github-actions bot added the area/platform issues related to the platform label Sep 1, 2022
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

I have questions about the intent here. This seems like a lot of engineering for something that doesn't seem like a simple problem.
Do we only want to report certain pages? Is there some reason we couldn't parse out what we need from the url? React-router has useRouteMatch

@timroes
Copy link
Collaborator

timroes commented Sep 1, 2022

@krishnaglick The advantage of having it distrbuted is, that we don't need to have one central place (as of today), that needs to know about all the paths that exist across the app and handle that centrally (and therefore likely to forget adjusting that place when adding a new page). I personally prefer the approach of having it tracked in the page component itself. (I'd only have preferred more if we could have attached meta information to the actual react-router route and read that out, but unfortunately that's not supported by react-router - despite many people already asked for it).

@letiescanciano
Copy link
Contributor Author

@krishnaglick thanks for your comment. As Tim was explaining, we see benefits on having it colocated directly at the page level, making the page component responsible for its own tracking.
Re: inferring it from the url, we could indeed, but we want to have a more friendly page view name to make analytics easier. Segment will capture the url no matter what, but this way we can easily group page views for single items, for example, /workspaces/:id/source/:sourceId without having to worry about ids when we want to analize the data.

@krishnaglick
Copy link
Contributor

@krishnaglick thanks for your comment. As Tim was explaining, we see benefits on having it colocated directly at the page level, making the page component responsible for its own tracking. Re: inferring it from the url, we could indeed, but we want to have a more friendly page view name to make analytics easier. Segment will capture the url no matter what, but this way we can easily group page views for single items, for example, /workspaces/:id/source/:sourceId without having to worry about ids when we want to analize the data.

I guess this just sets off my code smell, but I understand. Having that single data point to query against would be easier on analytics.

@@ -0,0 +1,33 @@
export const PAGE_TRACKING_CODES = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We tend to use enums for those places we plan to have "complete" lists of keys that should be used. Also we'd be able to change then the type of useTrackPage parameter type to only accept that enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also could we maybe re-export this type from the src/hooks/services/Analytics/index.ts file, so we can import it from there. We try to re-export types and methods that are supposed to be used outside that specific folder, and if it's not re-exported try to treat it as private to this folder.

* master: (47 commits)
  Add email to identify users analytics call (#16327)
  馃帀 Source Amazon Ads: improve `config.start_date` validation (#16191)
  Add comments about intermediate state emission (#16262)
  MySQL Source : Standardize spec.json for DB connectors that support log-based CDC replication (#16216)
  MSSQL Source : Standardize spec.json for DB connectors that support log-based CDC replication (#16215)
  Hide a bunch more destination with potential unsecure API access (#16320)
  Skip unit tests when run-tests is false (#16267)
  Hide Destination connections from UI (#16310)
  Add scheduled task to clean up old files from workspace (#16247)
  Source Google Analytics v4: Re-name google analytics connector (#16306)
  馃悰 Source Facebook Marketing: remove "end_date" from config if empty value (re-implement #16096) (#16222)
  Fix github action syntax (#16277)
  Re-name google analytics cionnectors (#16287)
  Bump Airbyte version from 0.40.3 to 0.40.4 (#16275)
  Hide ES and Redis destination connectors from Cloud (#16276)
  15700  add tests for PokeAPI (#15701)
  Add ProtocolVersion to StandardDefs (#16237)
  馃獰 馃敡 馃Ч Migrate attempt `bytesSynced` to `totalStats.bytesEmitted` and cleanup `AttemptDetails` component (#16126)
  Improve behavior of password input field (#16011)
  Improve airbyte-metrics support in the Helm chart (#16166)
  ...
Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM, have not tested locally. Vale!

@letiescanciano letiescanciano merged commit bdfafe5 into master Sep 5, 2022
@letiescanciano letiescanciano deleted the leti/fix-page-tracking-in-segment branch September 5, 2022 11:28
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* 馃獰 馃敡 Add proper page view events for Segment
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* 馃獰 馃敡 Add proper page view events for Segment
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 area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment page calls not firing on Cloud
3 participants