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

perf: Enable property group optimizations for all new teams by default #25010

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

tkaemming
Copy link
Contributor

@tkaemming tkaemming commented Sep 16, 2024

Problem

New teams still use the slower path even though they don't have any data that needs backfilling.

Changes

This changes the default value of modifiers so that all teams use the new path (see: #24152, #24171, #24381, #24897 for details.)

This could also be done with a feature flag but this has a couple advantages:

  • we're already using modifiers in production for this, so change is minimal and avoids adding new layers of configuration (like what happened with the PoE rollout)
  • allows hobby deployments to use the new functionality for new projects without any extra work beyond the typical "run migrations" step

It does have some disadvantages though, too:

  • can't do double duty as a kill switch in an emergency
  • it is less auditable who made changes and when
  • includes a migration with a clumsy import back into application code

I couldn't really make up my mind on which approach was "best" so just defaulted to the least impact change since the likelihood that we'll need a kill switch seems minimal (we've been testing this for a while — and worst case we can just run an UPDATE to drop the modifier for all teams if we have to) and most if not all teams should be able to be migrated to this in the coming days. (After this is merged, I'm planning on updating all teams that were created after we started writing events with property groups to use this optimization, in fact.)

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

Yes: see above, this turns it on for new projects in self-hosted as well.

How did you test this code?

See updated snapshots.

@tkaemming tkaemming force-pushed the property-groups-optimized-default branch from 2b56481 to 929512b Compare September 17, 2024 17:32
@tkaemming tkaemming force-pushed the property-groups-optimized-default branch from 929512b to 28449df Compare September 17, 2024 17:51
@tkaemming tkaemming requested review from timgl and a team September 17, 2024 22:57
@tkaemming tkaemming marked this pull request as ready for review September 18, 2024 02:55
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.

1 participant