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

Insights data instrumentation #2787

Merged
merged 15 commits into from Jan 14, 2021
Merged

Insights data instrumentation #2787

merged 15 commits into from Jan 14, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Dec 16, 2020

Changes

Adds some key instrumentation related to insights.

  • Introduces insight viewed event which is triggered when a user visits an insights graph (trends, sessions, retention ...).
    • The event is also triggered anytime the filtering properties change.
    • The event is de-bounced for 500ms. This is just to avoid noisy events from a user changing the filters multiple times in a row.
    • The event supports both states for the remove-shownas feature flag introduced on Remove shownas filter and move stickiness/lifecycle into separate insight tabs #2899. Regardless of the state of the feature flag, the insight property will be set to TRENDS, LIFECYCLE or STICKINESS as appropriate.
  • Introduces dashboard viewed event which is triggered when a user visits a dashboard.

Please review the code for context on all the included metadata on each tracked event.

Jan 12 update
Per discussion with @macobo & @jamesefhawkins, we've removed the requirement for the user to stay in the page for 5s before firing the event. This to avoid adding complexity that brings little value, and the 5s rule can be arbitrary (i.e. value can be driven by looking at a graph for less than 5 seconds and conversely, staying 5s does not guarantee driving value).

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-instrumentation-0f5s8v December 16, 2020 03:56 Inactive
@timgl timgl temporarily deployed to posthog-instrumentation-0f5s8v December 16, 2020 20:36 Inactive
@timgl timgl temporarily deployed to posthog-instrumentation-0f5s8v December 16, 2020 20:49 Inactive
@timgl timgl temporarily deployed to posthog-instrumentation-0f5s8v December 16, 2020 20:50 Inactive
@paolodamico paolodamico marked this pull request as ready for review December 17, 2020 01:34
@timgl timgl temporarily deployed to posthog-instrumentation-0f5s8v December 17, 2020 01:34 Inactive
@paolodamico
Copy link
Contributor Author

Tagging @jamesefhawkins in case you have any comments on the metadata we're attaching.

To whomever reviews the code: as this will be used for key metrics, can you help me dive into the weeds to be as certain as possible that we're capturing these events correctly?

Hopefully we can merge on cloud before the final code freeze.

Copy link
Contributor

@macobo macobo left a comment

Choose a reason for hiding this comment

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

Noticed some problems re ts - happy to help iron these out. :)

Change is 👍

frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/dashboard/dashboardLogic.js Outdated Show resolved Hide resolved
frontend/src/scenes/dashboard/dashboardLogic.js Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-instrumentation-0f5s8v December 17, 2020 16:25 Inactive
@paolodamico
Copy link
Contributor Author

Thanks for the thorough review @macobo addressed all your feedback. Re filters typing, agreed on taking small steps towards improving typing here, starting with this new action payload! As we make sure it captures all cases we can use this same interface for the general filters.

Ready for a quick final look if you want too. I also forgot to ask if you got a chance to actually test the functionality and make sure we're triggering the events at the right time.

@EDsCODE
Copy link
Member

EDsCODE commented Dec 17, 2020

Also heads up, typing the filters is on mine and Tim's release cycle. We haven't prioritized it so far because we've been doing more of the backend fixes but can pick up wherever you leave it

@paolodamico
Copy link
Contributor Author

Re @EDsCODE good to know! Hope this gives you a bit of a head start

@macobo
Copy link
Contributor

macobo commented Dec 17, 2020

Thanks for doing the changes! Looking great, I wouldn't mind if you just went ahead and merged it. I did not do any manual testing (yet) - can do so tomorrow if you want to wait!

@paolodamico
Copy link
Contributor Author

Thanks @macobo! I actually prefer to wait to make sure this is as accurate as possible as it'll be used for key metrics. I don't think we should merge anything new anyways.

@paolodamico
Copy link
Contributor Author

@macobo ready for another view, I've updated according to your comments on #2811 and what we discussed yesterday. PR description updated too.

If you could help testing too, it'd be incredibly helpful.

frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/insights/insightLogic.ts Outdated Show resolved Hide resolved
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Karl caught many of the details. My thoughts resulting from his comments are that it might be worth having an individual logic for analytic events? The reportusage action can just exist separately and be injected when needed. This will reduce the confusion of app code and analytics code.

@paolodamico
Copy link
Contributor Author

Thanks for the feedback, I've addressed all of it and also per @EDsCODE's suggestion, moved all the analytics logic to the eventUsageLogic component.

@paolodamico paolodamico merged commit 39270b3 into master Jan 14, 2021
@paolodamico paolodamico deleted the instrumentation-d15 branch January 14, 2021 16:59
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