perf(workflows): Avoid ~0.6s regression in backported OrganizationCombinedRuleIndexEndpoint.get#111692
perf(workflows): Avoid ~0.6s regression in backported OrganizationCombinedRuleIndexEndpoint.get#111692
Conversation
…binedRuleIndexEndpoint.get
Backend Test FailuresFailures on
|
ceorourke
left a comment
There was a problem hiding this comment.
Approving based on your explanation and confidence in your analysis with the caveat that I am not that well versed in the Django internals. Feel free to wait for Dan's final approval.
| # Note: Monitor lives on a different database, so we can't use a transaction here | ||
| # (SET LOCAL would require one). Session-level SET + RESET is fine for a request-scoped | ||
| # connection. | ||
| with _postgres_jit_disabled(using=router.db_for_write(Detector)): |
There was a problem hiding this comment.
Why can't we use a transaction here? It won't apply to the crons db of course, but we can start a transaction in the default db and then just disable jit using local. That reduces the risk of us accidentally leaking this by setting it at the connection level
There was a problem hiding this comment.
As you can see in the test failures above, it runs afoul of enforce_no_cross_transaction_interactions (
Lines 94 to 99 in 28331f1
There was a problem hiding this comment.
That's fine, you can disable that. We unfortunately have to do this a lot when working with monitors, with in_test_hide_transaction_boundary():.
The JIT is expensive and being used when we don't need it. We can try to work around it or restructure the query to avoid the jit threshold, but just scoped disabling gets us exactly what we want.
I've confirmed the JIT impact by analyzing sample queries and observing that the vast majority of the time is spent in JIT, but with JIT disabled the query is much faster.
Demo at https://redash.getsentry.net/queries/10853
I've also confirmed in traces that this query is consistently slow (280ms-450ms) and issued twice per request.
NB: The workflow engine variant of this endpoint isn't yet released, so the risk here should be quite low.