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

Instrument toolbar #2859

Merged
merged 12 commits into from Jan 7, 2021
Merged

Instrument toolbar #2859

merged 12 commits into from Jan 7, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Jan 5, 2021

Changes

Introduces basic instrumentation to the toolbar.

  • Introduces the toolbar mode triggered event which is fired when any of the toolbar modes (heatmap, inspect, stats, ...) are activated/deactivated.
  • Introduces the toolbar selected HTML element event which is fired when an element in the host's website is clicked for further details.
  • Adds UTM tags to the help url for the toolbar button so we can track on the docs when this button is clicked.
  • Introduces the toolbar dragged event which is fired after the toolbar has been dragged and is dropped somewhere in the screen.
  • Introduces the toolbar loaded event which is fired when the toolbar component is loaded.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 5, 2021 21:56 Inactive
@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 5, 2021 23:20 Inactive
@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 5, 2021 23:25 Inactive
@paolodamico
Copy link
Contributor Author

@mariusandra when you get a chance to review this would you mind making sure all the events are properly triggered? Want to make sure we don't have weird events coming in that affect our metrics. I also ran into an issue sending events with posthog.capture at some point, but I think that was my computer.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

The big issue with this PR is that you're using posthog-js, which won't work here. Instead you should import { posthog } from '~/toolbar/posthog' and capture via that.

I'm not sure if posthog-js would work now or not, but the original issue (and the reason for writing posthog-js-lite in the first place) was that I could not easily include two copies of posthog-js on a page at the same time. The toolbar is always running on a site that already has a copy of posthog-js running and sending events for the client. We need to be careful not to send the "toolbar button clicked" events to the client, but to ourselves.

I'll see what can be done regarding the other questions...

frontend/src/toolbar/button/toolbarButtonLogic.ts Outdated Show resolved Hide resolved
frontend/src/toolbar/elements/elementsLogic.ts Outdated Show resolved Hide resolved
@mariusandra mariusandra temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 11:32 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 11:38 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 11:41 Inactive
@mariusandra
Copy link
Collaborator

I updated the posthog library to the right one, fixed some type errors and added information about the action and heatmap counts. It's probably OK to merge now, but please test :)

@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 20:55 Inactive
@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 21:10 Inactive
@timgl timgl temporarily deployed to posthog-toolbar-instrum-wvpd0u January 6, 2021 21:57 Inactive
@paolodamico
Copy link
Contributor Author

paolodamico commented Jan 6, 2021

Thanks for the review and suggestions, made some additional changes. Looks good to merge to me, but feel free to review once more before merging (will merge tomorrow otherwise). Also worth noting, noticed that we don't self-capture toolbar events in development, but keeping that for a separate PR.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks good!

@mariusandra mariusandra merged commit f9a35e2 into master Jan 7, 2021
@mariusandra mariusandra deleted the toolbar-instrumentation branch January 7, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants