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

feat: track anonymous personfull events in usage reports #23461

Merged

Conversation

raquelmsmith
Copy link
Member

Problem

We want to see how many of our events per customer are anonymous but personfull to see if we can make the anonymous events all personless.

Changes

In usage reports, adds a metric for anonymous personfull events.

I will re-run usage reports for the last few days so this metric shows up and we can run numbers from there.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

Updated the tests, which annoyingly took forever, I think there are too many ways to do the same thing, or they're named poorly or something.

@raquelmsmith raquelmsmith requested a review from robbie-c July 3, 2024 20:56
WHERE timestamp between %(begin)s AND %(end)s
AND event != '$feature_flag_called' AND event NOT IN ('survey sent', 'survey shown', 'survey dismissed')
AND person_mode IN ('full', 'force_upgrade')
AND JSONExtractBool(properties, '$is_identified') = 0
Copy link
Member Author

Choose a reason for hiding this comment

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

@robbie-c what if someone had used $set to add person properties - would they be considered identified from posthog.js?

@raquelmsmith raquelmsmith requested a review from zlwaterfield July 3, 2024 22:59
Copy link
Contributor

@zlwaterfield zlwaterfield left a comment

Choose a reason for hiding this comment

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

Just added two questions

@@ -224,11 +224,12 @@ def bulk_create_events(

except Group.DoesNotExist:
continue
properties = event.get("properties", {})
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seems like it didn't anything.

@@ -74,6 +74,7 @@ class UsageReportCounters:
event_count_in_period: int
enhanced_persons_event_count_in_period: int
event_count_with_groups_in_period: int
anonymous_personfull_event_count_in_period: int
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: shouldn't this have one l? "personful"

Copy link
Member Author

Choose a reason for hiding this comment

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

We've used "personfull" in internal communications but I think it's a britishism, so yes changing to personful 😄

@raquelmsmith raquelmsmith enabled auto-merge (squash) July 5, 2024 23:38
@raquelmsmith raquelmsmith merged commit d302688 into master Jul 6, 2024
84 checks passed
@raquelmsmith raquelmsmith deleted the feat/rms/usage-reports-would-have-been-personless branch July 6, 2024 00:13
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