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

fix(trends): fix active users persons date range #17356

Merged
merged 14 commits into from
Sep 11, 2023

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Sep 7, 2023

Problem

The persons response for active users, i.e. WAU and MAU, is off. See these tickets https://posthoghelp.zendesk.com/agent/tickets/5313, https://posthoghelp.zendesk.com/agent/tickets/5126.

Changes

As often, understanding what is going on is the hardest part of solving the problem:

  • Calculation of active users: To calculate active users we take the cartesian product of eligible events* and dates for which we want to know the active users, then filter out rows where event.timestamp <= start_of_day + 1 day and event > start_of_day - 6 days (or - 29 for MAU). Grouping the rows by start_of_day yields the active users count.
  • Building the persons url: The persons url for active users is built on the backend, whereby
    • date_from would be start_of_day and
    • date_to would be offset_time_series_date_by_interval(start_of_day), resulting in
      • end_of_day for a daily interval and
      • start_of_day + 6 days for weekly interval (etc. for other intervals)
  • Building the final filters: The date_from is adjusted by the get_active_user_params method, by subtracting 7 / 30 days.

(*) there is actually a bug due to which the first interval doesn't receive all events it should #14063

This PR fixed the calculation by offsetting the date_from by one day. A skipped test case is added for the breakdown case, where it is not yet fixed (also do to another issue with the breakdown case, where values would not be displayed for all dates likely due to a wrong base for the aforementioned cross product). The goal now is to instead convert existing insights to HogQL and fix the remaining edge cases then.

How did you test this code?

Manually sent test events, with a separate user for each day:

curl -v -L --header "Content-Type: application/json" -d '{
  "api_key": "<ph_project_api_key>",
  "event": "my_wau_event",
  "distinct_id": "day_31_08_2023",
  "timestamp": "2023‐08‐31T13:37:42Z"
}' http://localhost:8000/capture/

Then tested responses with a deactivated cache.

@thmsobrmlr thmsobrmlr closed this Sep 7, 2023
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 like black magic. Here's my LGTM if you want a TGIF.

@thmsobrmlr thmsobrmlr enabled auto-merge (squash) September 11, 2023 11:57
@thmsobrmlr thmsobrmlr merged commit 7f24c42 into master Sep 11, 2023
56 checks passed
@thmsobrmlr thmsobrmlr deleted the fix-active-users-persons branch September 11, 2023 12:23
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