feat(aci): SlowDbQueryDetectors #103136
Open
feat(aci): SlowDbQueryDetectors #103136
+1,100
−21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Scope: Frontend
Contributor
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
+31
to
+33
| id__in=Detector.objects.filter(type=SLOW_DB_QUERY_TYPE_SLUG).values_list( | ||
| "project_id", flat=True | ||
| ) |
There was a problem hiding this comment.
High severity vulnerability may affect your project—review required:
Line 31 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
- If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| for project in RangeQuerySetWrapper(projects_with_slow_db_detection): | ||
| try: | ||
| with transaction.atomic(router.db_for_write(Project)): |
Contributor
There was a problem hiding this comment.
Bug: Database Router Mismatch for Workflow Engine Models
Using router.db_for_write(Project) for a transaction that creates Detector and DataConditionGroup objects from the workflow_engine app. The router should use router.db_for_write(Detector) to ensure the transaction uses the correct database for workflow_engine models.
| # Resolve validator if it's a callable factory function | ||
| validator = type.detector_settings.validator | ||
| if callable(validator) and not isinstance(validator, type): | ||
| validator = validator() |
Contributor
There was a problem hiding this comment.
Bug: Shadowed type breaks validation.
Variable type shadows the built-in type, causing isinstance(validator, type) to check against the GroupType instance instead of checking if validator is a class. This causes validator classes to be incorrectly instantiated when passed directly. Should use a different variable name or reference the built-in type explicitly.
| # Resolve validator if it's a callable factory function | ||
| validator = type.detector_settings.validator | ||
| if callable(validator) and not isinstance(validator, type): | ||
| validator = validator() |
Contributor
There was a problem hiding this comment.
Bug: Shadowed type variable: Class identity crisis.
Variable type shadows the built-in type, causing isinstance(validator, type) to check against the GroupType instance instead of checking if validator is a class. This causes validator classes to be incorrectly instantiated when passed directly. Should use a different variable name or reference the built-in type explicitly.
❌ 79 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
|
This PR has a migration; here is the generated SQL for for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backend migration for slow db queries and API compatibility layers with project options
This does a few things:
A migration strategy for detector settings
We convert the project settings that power legacy performance detectors into actual detector models with configuration values. We prefer reading from the detector values if they exist. This makes detectors appear in the new UI and allow editing their configuration values via the ACI interface. It is backward compatible with the old project options APIs as well.
The approach should mostly "just work" as we migrate over the remaining performance detectors.
A dynamically rendered UI powered by detector json schema configurations
This will work for all future performance detectors and probably others in the future. We may want to add a few more bells and whistles to explain the various detectors. That's left as future work.
Updates the detector-types API
This wasn't used afaict. Now it is and it powers these new UIs.
It does not do other things that we will want in the future: