Skip to content

ref(workflows): Cache assignee lookup in-memory#115863

Open
cmanallen wants to merge 6 commits into
masterfrom
cmanallen/workflow-engine-batch-caching
Open

ref(workflows): Cache assignee lookup in-memory#115863
cmanallen wants to merge 6 commits into
masterfrom
cmanallen/workflow-engine-batch-caching

Conversation

@cmanallen
Copy link
Copy Markdown
Member

@cmanallen cmanallen commented May 19, 2026

Its the same lookup on repeat. We can reuse the results from the previous iteration.

@cmanallen cmanallen requested a review from a team as a code owner May 19, 2026 23:31
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 19, 2026
@cmanallen cmanallen marked this pull request as draft May 19, 2026 23:51
@kcons
Copy link
Copy Markdown
Member

kcons commented May 20, 2026

Yeah, this is a known perf issue in our condition evaluation that we've got special-cased for Snuba-dependent conditions but mean to eventually generalize; ideally, we could bulk gather required data, then run an evaluation phase.
If it's causing issues I'm certainly not against mitigating it before we get around to fixing it structurally, but I'd feel comfortable with a bit more structure, like a doc on _cache and maybe a TypedDict showing the shape of the data. Or, keying it by a private module level object() so the data is borderline unreachable by any other means. WorkflowEventData is used in lots of places and is really intended to be immutable, so I don't want to make it a bag-of-vals without some safeguards. Or maybe this is a case for local_cache()? Nobody seems to use that, but if roundtrips to memcache are a problem, that seems build for that.

@cmanallen
Copy link
Copy Markdown
Member Author

cmanallen commented May 20, 2026

@kcons

If it's causing issues I'm certainly not against mitigating it before we get around to fixing it structurally,

No issues per se. I was looking at the top span emitters by count (and duration) and I spotted the trace I linked above. Figured I'd fix it rather that leave it.

As for structure, sure I can do that. The goal was to be minimally intrusive so I wouldn't exhaust potential collaborators. We can do whatever here. If you have a preference let me know. Adding stronger typing is certainly an easy value add.

@cmanallen cmanallen marked this pull request as ready for review May 20, 2026 00:34
@kcons kcons self-requested a review May 21, 2026 17:22
@cmanallen
Copy link
Copy Markdown
Member Author

@kcons Any thoughts?

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.

2 participants