Skip to content

fix(workflows): add detector group caching in ensure_association_with_detector#111714

Merged
klochek merged 1 commit intomasterfrom
christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector
Apr 2, 2026
Merged

fix(workflows): add detector group caching in ensure_association_with_detector#111714
klochek merged 1 commit intomasterfrom
christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector

Conversation

@klochek
Copy link
Copy Markdown
Contributor

@klochek klochek commented Mar 27, 2026

No description provided.

@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 27, 2026

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 27, 2026
@klochek klochek changed the title fix(workflows): add detector group caching in ensure_association_with… fix(workflows): add detector group caching in ensure_association_with_detector Mar 27, 2026
Comment thread src/sentry/workflow_engine/processors/detector.py Outdated
@github-actions

This comment was marked as resolved.

@klochek klochek force-pushed the christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector branch from 2afc4f8 to b4ee696 Compare March 27, 2026 19:29
@github-actions

This comment was marked as resolved.

@klochek klochek force-pushed the christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector branch from b4ee696 to dca5cbc Compare March 27, 2026 19:56
@github-actions

This comment was marked as resolved.

@klochek klochek force-pushed the christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector branch from dca5cbc to a4b0d88 Compare March 30, 2026 14:22
@klochek klochek force-pushed the christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector branch from a4b0d88 to c06f337 Compare March 30, 2026 15:01
@klochek klochek marked this pull request as ready for review March 30, 2026 15:34
@klochek klochek requested a review from a team as a code owner March 30, 2026 15:34
@klochek klochek requested a review from saponifi3d March 30, 2026 15:34
@kcons kcons self-requested a review March 30, 2026 16:56
# Common case: it exists, we verify and move on.
if DetectorGroup.objects.filter(group_id=group.id).exists():
try:
DetectorGroup.objects.get_from_cache(group=group)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any sense of expected hit rate here? Even just "avg num events / avg num groups" or something would be interesting. Not a requirement, just curious..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No idea--it's a pretty hot query according to datadog, though, so tracking this to see if there's improvement should be easy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is default ttl 60min?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, at this point this method is probably a no op. every error group should be created with a Detector Group. maybe we can also call it dramatically less

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

agree that we could probably reduce the number of times we're calling this method, should that be a separate ticket / investigation so we can merge this and reduce the DB load while we investigate a more holistic solution?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah yes, sorry, I lost track of this because my pr tracking filtered it for not indicating it needs review, will fix.

To close the loop on usage, current data shows us typically getting value out of this code 0% of the time, and peaking at around 0.00049% of the time. Which is to say, at this point, new groups basically always have a DetectorGroup.
We see a handful per day (9 in datadog typically, but we can't go lower) and yesterday was the only spike this month with ~200 (out of >250m).
So, I guess this code is useful for turning "basically always" to "always", though I'm not sure if that's worth enough to be running this as often as we are, but caching sure helps either way (assuming a decent hit rate).

Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

lgtm as a way to reduce load on the DB -- it'd be cool if we could determine a way to remove or reduce this being called though. mind also adding a backlog ticket so we can track this work for after launch? (can add it to the aci backlog milestone)

@klochek klochek merged commit ee6bf90 into master Apr 2, 2026
107 checks passed
@klochek klochek deleted the christopherklochek/iswf-2310-cache-detector-groups-in-ensure_association_with_detector branch April 2, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants