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(insights): Warn about WAU/MAU in total value trends #21067

Merged
merged 13 commits into from
Mar 26, 2024

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Mar 21, 2024

Problem

ZEN-11743 showcases how confusing and prone to errors the combination "Weekly/Monthly active users" math with total value trends is. For example, when you have a date range that's "Last 90 days", what does "Monthly active users" mean? There really is no intuitively correct answer that I can think of, so we should at the very least discourage this combination.

Changes

Added a warning for WAU/MAU series on total value trends:

warn

How did you test this code?

Added an E2E test.

@Twixes Twixes requested a review from a team March 21, 2024 11:46
WorldMap = 'WorldMap',
BoldNumber = 'BoldNumber',
}
export enum ChartDisplayCategory {
Copy link
Member Author

@Twixes Twixes Mar 21, 2024

Choose a reason for hiding this comment

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

Added ChartDisplayCategory as an abstraction over the specific display types

I also wanted to use it in the backend, the way we reuse ChartDisplayType thanks to schema.{ts,json,py}, but unfortunately ts-json-schema-generator doesn't support re-exporting, so we can't do that (unless we start using ChartDisplayCategory in a type defined in schema.ts itself). I put out a fix for the library (super interesting opportunity to learn more about TS syntax internals!), but in the meantime it's fine for this to be frontend-only

Copy link
Member Author

Choose a reason for hiding this comment

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

ChartDisplayCategory would specifically be super useful for caching

Copy link
Contributor

github-actions bot commented Mar 21, 2024

Size Change: +261 B (0%)

Total Size: 824 kB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 824 kB +261 B (0%)

compressed-size-action

@thmsobrmlr
Copy link
Contributor

Might it be better if we hide these two items for total value insights? Or other way around: When would I use the combination of these two? Right now I find the warning a bit strongly worded and would go for something like "in most cases you want to use unique users, only for ... it makes sense to use wau/mau".

@Twixes
Copy link
Member Author

Twixes commented Mar 21, 2024

Hmm, hiding is the trickier option, because people switch between time series and total value all the time (like the user in the ticket), so the solution wouldn't be complete without accounting for that workflow. So either:

Option 1: We only hide MAU/WAU on total insights if not already used – but then we still need the warning to inform about the problem

Option 2: We switch from MAU/WAU to "Unique users" for the user – only the results would still be unexpected, because there's the same problem: what date range should MAU/WAU mean in a total value context? I think the only solution that isn't surprising would be a modal on display type change to "Hey, what date range do you want for Unique users specifically?" But this also breaks down when there are multiple series where not all are MAU/WAU.

Hence leaning towards just the warning for now.

I think what'd be amazing (once HogQL trends are the only trends) would be a series-level rolling average option to replace both WAU/MAU and the current "smoothing" feature. Basically this: #11716

Comment on lines 579 to 580
In total value insights, it's usually not clear what date range "{definition.name}" refers
to. For full clarity, we recommend using "Unique users" here instead
Copy link
Member Author

Choose a reason for hiding this comment

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

I reworded this a bit to hopefully suggest "Unique users" more gently @thmsobrmlr The tricky bit is that it's really hard to understate how confusing MAU/WAU is in this situation 😅

@thmsobrmlr
Copy link
Contributor

Hmm, hiding is the trickier option, because people switch between time series and total value all the time (like the user in the ticket), so the solution wouldn't be complete without accounting for that workflow. So either:

Option 1: We only hide MAU/WAU on total insights if not already used – but then we still need the warning to inform about the problem

Option 2: We switch from MAU/WAU to "Unique users" for the user – only the results would still be unexpected, because there's the same problem: what date range should MAU/WAU mean in a total value context? I think the only solution that isn't surprising would be a modal on display type change to "Hey, what date range do you want for Unique users specifically?" But this also breaks down when there are multiple series where not all are MAU/WAU.

Hence leaning towards just the warning for now.

Yup, that makes sense. One thought I had: We could have a general warning area of some form for insights, since people might easily miss the warning in the dropdown when switching between time series and total value insights. We could even add an ignoredWarning prop to the query nodes so that warnings can be dismissed as well. Nothing for now of course, just an idea since cases like this come up quite frequently.

I think what'd be amazing (once HogQL trends are the only trends) would be a series-level rolling average option to replace both WAU/MAU and the current "smoothing" feature. Basically this: #11716

Oh yeah, that looks good. Didn't know about this suggestion at all.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit 93f47ad into master Mar 26, 2024
134 checks passed
@Twixes Twixes deleted the warn-about-trailing-count-in-total-value branch March 26, 2024 11:28
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.

3 participants