-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: Add assertMaxWriteQueries to test max queries against a table #8446
Conversation
src/sentry/testutils/cases.py
Outdated
real_queries = {} | ||
|
||
for query in self.captured_queries: | ||
match = re.search(r"u'([^']*)'", query['sql']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this use ast.literal_eval
:
>>> import ast
>>> ast.literal_eval('u"Hello World!"')
u'Hello World!'
What’s wrong with just the builtin assertNumQueries? I get that this provides a bit more behavior, but seems a little overkill imo. I don’t think we typically care which tables, and read vs write usually. It’s just as easy to introduce a bunch of extra read queries that cause a similar incident. |
@mattrobenolt easier to debug because you can see what the extra query is that was not expected before. FWIW I did not make good experiences in the past with just query counting in tests as you typically just define a max and then refactoring just invalidates the point of it if they drop or caching changes some assumptions. I would almost like to fully sign off on all the queries but that seemed like a good middleground. |
How does this work with JOINs and subqueries, etc? Which table are they counted against? All of them? One for each? |
The only other comment I have with this is why explicitly only testing for write queries? What about read queries? I think these are all valid since it's more likely that we introduce unintended read queries. |
@mattrobenolt The library we use for parsing the query doesn't really work reliable for subqueries/joins to count the "reads". So we could invest more time to make it work by parsing the queries manually to correctly get the "read" count but not sure if we should really do this now. |
This PR adds a context method for unit tests to check against write queries on tables called:
assertMaxWriteQueries
Usage:
This checks for
UPDATE
INSERT
DELETE
queries and counts how often they will be executed in a single unit test.While this this approach is very basic, it would have caught the outlier in queries responsible for the latest outage.
See example:
If we re-add the line that was responsible for the outage again:
The test fails with:
This means in the future if someone introduces more "write" queries to the event ingestion, we would notice it upfront.