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: Separate OR clauses from Insights counts #16502

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

keithwillcode
Copy link
Contributor

@keithwillcode keithwillcode commented Sep 5, 2024

What does this PR do?

Similar to the approach taken in #16398 and #16410, we are breaking down the OR clauses that cause Seq Scan on the Booking and EventType tables. This will instead run separate queries which properly utilize the indices and result in Index Scan instead.

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works. - We have decided that tests will be added to Insights after we verify this path forward will indeed fix the perf issues.

How should this be tested?

  • Test insights to ensure the results of the queries have not changed.

Copy link

vercel bot commented Sep 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2024 1:31pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2024 1:31pm

@keithwillcode keithwillcode requested a review from a team September 5, 2024 13:21
@keithwillcode keithwillcode added this to the v4.5 milestone Sep 5, 2024
@keithwillcode keithwillcode added the High priority Created by Linear-GitHub Sync label Sep 5, 2024
@keithwillcode keithwillcode self-assigned this Sep 5, 2024
@keithwillcode keithwillcode marked this pull request as ready for review September 5, 2024 13:21
@graphite-app graphite-app bot requested a review from a team September 5, 2024 13:21
@dosubot dosubot bot added the insights area: insights, analytics label Sep 5, 2024
},
});
let data;
if (!!where["OR"]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this function is a helper to all of insights, creating this logic here to separate out the ORs is a much simpler change than going through every place where we run queries and splitting the logic to run multiple queries. i.e. This is more foundational logic being changed. Upon seeing this work well in canary and prod, the idea would be to pull this out into a generic helper that we can use wherever we want, which will then be used to update all of the places in trpc-router.ts that use slow or clauses.

Copy link
Member

Choose a reason for hiding this comment

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

Fan of this 🙌

Copy link

graphite-app bot commented Sep 5, 2024

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (09/05/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

github-actions bot commented Sep 5, 2024

E2E results are ready!

},
});
let data;
if (!!where["OR"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Fan of this 🙌

@keithwillcode keithwillcode merged commit 458e6a3 into main Sep 5, 2024
39 of 40 checks passed
@keithwillcode keithwillcode deleted the perf/improve-insights branch September 5, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core area: core, team members only foundation High priority Created by Linear-GitHub Sync insights area: insights, analytics ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants