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

Add PostHog analytics #124

Merged
merged 11 commits into from
Jul 11, 2024
Merged

Add PostHog analytics #124

merged 11 commits into from
Jul 11, 2024

Conversation

nichochar
Copy link
Contributor

@nichochar nichochar commented Jul 11, 2024

This PR introduces posthog analytics for Srcbook. It's an opt-out mechanism, but we don't collect any PII data. Location tracking is disabled, and the only metrics we collect are behavioral and explicit, in the goal of improving the Srcbook application.

The user can disable tracking in settings.

I added some copy about this in the README. Down the line we might want a more formal document about the data we collect.

I collect on the node side of things (rather than the web side of things). This is a debatable option. It's great for source of truth, but won't help us understand things like clicks / pageViews, etc... It has the advantage of not being able to be blocked by ad-blockers, and won't show up in the network tab.

I do not collect in dev (I guard on NODE_ENV === production)

Testing

This was tested manually, checking that it behaves correctly when enabled and disabled.

Events

To begin, we track the following events:

  • session create (with TS/JS metadata)
  • generate with AI
  • added openAI API key (generic "changed a setting" event, obviously we don't track the value)
  • delete Srcbook
  • export Srcbook

@nichochar nichochar requested a review from benjreinhart July 11, 2024 00:11
@nichochar nichochar changed the title Add Posthog analytics Add PostHog analytics Jul 11, 2024
Copy link
Contributor

@benjreinhart benjreinhart left a comment

Choose a reason for hiding this comment

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

Two points

  1. As far as I can tell, this will not work in production. We do not set NODE_ENV=production for the API in production (or if we do, I don't know where it happens).
  2. Aren't events conventionally defined as variable names instead of descriptions, like srcbook:started instead of "user started the srcbook application" ? In all the metrics products i used, there were short event names that acted as keys and you could associate descriptions with them, but that made it more consistent / less error prone to reuse events in a code base. Also, it feels like it would be easier to query in the analytics product?

<div>
<h2 className="text-xl pb-2">Analytics tracking</h2>
<label className="opacity-70 block pb-2">
We track behavior analytics to improve the Srcbook product. We track no personal
Copy link
Contributor

Choose a reason for hiding this comment

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

We track behavioral analytics to improve Srcbook. We do not track any personally identifiable information (PII).

@nichochar
Copy link
Contributor Author

On your 2 points:

  1. Hmm I used to set it in production when running the application, but clearly this got scrapped at some point. I will reintroduce it and fix.
  2. For event names, I'm directly following the recommendation from PostHog (see screenshot). Assuming they know the best practice since it's their core business (I also hadn't seen it before).
    CleanShot 2024-07-11 at 10 30 57

@@ -1,3 +1,7 @@
#!/usr/bin/env node

import '../index.mjs';
process.env.NODE_ENV = 'production';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use dotenv here, or some other more standard way? We're going to eventually want to set more flags in prod anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind if I do this in a follow up? This PR is already blown up a bit.

process.env.NODE_ENV = 'production';

(async () => {
await import('../index.mjs');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is using await here necessary? And if it is, why do we need to wrap it in a function? We use node with top-level await I thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about using await, but the dynamic import is important, otherwise it gets run before setting the process.env.NODE_ENV.

I don't think there's anything wrong with the current syntax?

@nichochar nichochar merged commit 95a885d into main Jul 11, 2024
1 check passed
@nichochar nichochar deleted the posthog-analytics branch July 11, 2024 19:25
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.

2 participants