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(experiments): only show supported math functions #27589

Merged
merged 20 commits into from
Jan 22, 2025

Conversation

andehen
Copy link
Contributor

@andehen andehen commented Jan 16, 2025

Problem

We currently show unsupported math functions. Choosing one of these will yield invalid results.

Changes

Only show supported math functions. That is, functions that return either a count or a sum.
Screenshot 2025-01-22 at 10 50 14

How did you test this code?

Tested locally

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Size Change: -766 B (-0.07%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB -766 B (-0.07%)

compressed-size-action

@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.

@andehen andehen force-pushed the experiments-remove-count-agg-options branch from 10b1620 to 1c5829b Compare January 16, 2025 15:09
@andehen andehen changed the title fix(experiments): remove unsupported math functions fix(experiments): only show supported math functions Jan 16, 2025
@andehen andehen marked this pull request as ready for review January 21, 2025 08:11
@andehen andehen requested a review from a team January 21, 2025 08:20
@jurajmajerik
Copy link
Contributor

Only show supported math functions. Work in progress to validate which ones are supported.

I don't get this 🤔

@jurajmajerik
Copy link
Contributor

For experiments that currently do have an unsupported metric type selected, this will somewhat break the UI.

image

Have we checked how many of such experiments there are?

@andehen
Copy link
Contributor Author

andehen commented Jan 21, 2025

Only show supported math functions. Work in progress to validate which ones are supported.

I don't get this 🤔

Yeah, specifically, I'm was not sure yet what to do with "Weekly active users", but it does make sense to include that one as well, and it should yield correct results. Updated the description.

@andehen
Copy link
Contributor Author

andehen commented Jan 21, 2025

For experiments that currently do have an unsupported metric type selected, this will somewhat break the UI.
Have we checked how many of such experiments there are?

Yeah, I have this Metabase question. There is in total ~ 42 metrics that will not be supported for running experiments. I think it's fine to accept that UI glitch for those.
Screenshot 2025-01-21 at 12 33 28

@andehen andehen force-pushed the experiments-remove-count-agg-options branch from 1fbb4c5 to 41b5248 Compare January 21, 2025 11:44
@@ -98,7 +98,15 @@ export function TrendsMetricForm({ isSecondary = false }: { isSecondary?: boolea
showSeriesIndicator={true}
entitiesLimit={1}
showNumericalPropsOnly={true}
onlyPropertyMathDefinitions={[PropertyMathType.Sum]}
allowedMathTypes={[
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 define this list in one place and use across both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in daa9008

@andehen andehen force-pushed the experiments-remove-count-agg-options branch from f25d672 to 4d41c00 Compare January 22, 2025 09:43
@andehen andehen merged commit 091e1b4 into master Jan 22, 2025
99 checks passed
@andehen andehen deleted the experiments-remove-count-agg-options branch January 22, 2025 13:37
fuziontech added a commit that referenced this pull request Jan 22, 2025
* master: (103 commits)
  feat(postgres-estimated-rows): pg Estimated Rows on Data Warehouse Sync (#27634)
  fix: revert darkmode class toggle, updated content on fills (#27783)
  chore: upgrade posthog-js (#27790)
  chore(editor-3001): add back join actions (#27740)
  feat: Add person distinct ID overrides squash job (as dagster job) (#27710)
  fix(created-by-sources): Adding `created_by` to sources (#27751)
  Revert "feat(data-warehouse): V2 pipeline release " (#27791)
  fix: typo for feature flags (#27786)
  fix(defer-unmounting): Defer unmounting of react elements (#27742)
  feat(data-warehouse): V2 pipeline release (#27732)
  fix(data-warehouse): Ensure dates are actual datetime formats (#27777)
  fix: enable hot reload for the products dir (#27746)
  fix: assignee selector when null (#27737)
  chore: clarify rrweb imports (#27776)
  chore(deps): Update posthog-js to 1.207.3 (#27779)
  feat(retention): filters on start/return event (#27770)
  fix(experiments): only show supported math functions (#27589)
  feat(web-analytics): Set unique conversions graph when adding conversions goal (#27774)
  chore: color design system part 1: banner and accents (#27756)
  chore(experiments): Add tests for funnel attribution options (#27752)
  ...
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.

4 participants