-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
impr(workflow engine): Add caching for detector lookup by data source #106085
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
base: master
Are you sure you want to change the base?
Conversation
saponifi3d
left a comment
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.
let me know if it'd help to hop on a call / discuss any of these comments too! this whole area is pretty confusing, even for those with context. haha
src/sentry/workflow_engine/endpoints/validators/base/detector.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/endpoints/validators/base/detector.py
Outdated
Show resolved
Hide resolved
saponifi3d
left a comment
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.
up to you on flattening the cache stuff and pulling it off of the detector model, i would recommend it as it will allow us to DRY up the code around purging / updating the cache.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
saponifi3d
left a comment
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.
🎉 this is looking great, mostly detailed comments on the implementation.
| Detector.objects.filter( | ||
| enabled=True, data_sources__source_id=source_id, data_sources__type=query_type | ||
| cache_access = DetectorByDataSourceCacheAccess(source_id, query_type) | ||
| detectors = cache_access.get() |
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.
can you also add a rollout flag here so we can easily enable / disable caching incase there are any unexpected issues or invalidations we haven't accounted for?
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.
This one is a little weird since I'm not sure if the tradeoff is worth it. This callsite has some indirection where in order to reference an organization we need to look up the DataSource -> Organization. That's a db query every time though so although it's here idk if it's worth it.
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Okay basically we're caching the bulk query and adapting the single detector query in the subscription processor work with that.
Cursor Bugbot reviewed your changes and found no issues for commit e31fc92