Skip to content

feat(apigateway): add separated async apigw package#115624

Merged
gi0baro merged 10 commits into
masterfrom
gi0baro/apigateway-async-dedicated-package
May 19, 2026
Merged

feat(apigateway): add separated async apigw package#115624
gi0baro merged 10 commits into
masterfrom
gi0baro/apigateway-async-dedicated-package

Conversation

@gi0baro
Copy link
Copy Markdown
Member

@gi0baro gi0baro commented May 15, 2026

SSIA

Depends on: getsentry/pypi#2155

@gi0baro gi0baro requested review from a team as code owners May 15, 2026 11:11
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 15, 2026
Comment thread src/apigw/proxy.py Outdated
Comment thread src/apigw/proxy.py Outdated
Comment thread src/apigw/proxy.py
Comment thread src/apigw/proxy.py
Comment thread src/apigw/proxy.py
Comment thread src/apigw/proxy.py
Comment thread src/apigw/proxy.py
@gi0baro gi0baro force-pushed the gi0baro/apigateway-async-dedicated-package branch from d22cffd to e1e5acd Compare May 15, 2026 16:45
Comment thread src/apigw/proxy.py
Comment thread src/apigw/views/proxy.py
Comment thread src/apigw/proxy.py
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a 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.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 246c26a. Configure here.

Comment thread src/apigw/proxy.py
Comment thread src/apigw/db.py
def pgq_from_djq(q: str, p: int) -> str:
for i in range(p):
q = q.replace("%s", f"${i + 1}", 1)
return q
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.

Django SQL %% escapes not unescaped for asyncpg

Low Severity

pgq_from_djq converts Django's %s placeholders to asyncpg's $N format but doesn't unescape %% to %. Django's sql_with_params() escapes literal % as %% (for psycopg2 compatibility). Since asyncpg doesn't interpret %%, any query containing literal % (e.g., LIKE patterns) would send doubled %% to PostgreSQL, producing incorrect results.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 246c26a. Configure here.

Comment thread src/apigw/proxy.py
@vgrozdanic
Copy link
Copy Markdown
Member

Can't we do the same with async Django view and async ORM?

I am a bit worried with introducing new dependencies just for this, if the same thing could be achieved with Django.

This will be hard to maintain in the long run, especially if you decide to move on or you no longer want to maintain emmett55 :)

Copy link
Copy Markdown
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

same as vjeran. why do we add this entirely separate service into this repo to begin with?

@gi0baro
Copy link
Copy Markdown
Member Author

gi0baro commented May 18, 2026

@vgrozdanic @untitaker sorry, the description lacks a lot of context.

We've already tried to use Django in ASGI with async views (implemented in #111307), but after ~1 mo of eval with a single pod, it turns out it's not feasible for production usage. Main issues observed in this timeframe were:

  • very high avg CPU load
  • unmanageable memory leaks (basically having workers reaching max RSS every minute)

This was mainly discussed with @getsentry/sre-production-engineering and @markstory, the idea is to launch a separated deployment with this reachable only from canaries with a specific set of headers, and monitor the actual behavior for few weeks before taking a decision on whether this is feasible or not.

The emmett55 choice doesn't have to be definitive here, it simply was the quickest way for me to have a MVP version ready to test (@vgrozdanic I've maintained this for the last 10+ years, don't see why I should stop). It can be relatively easy swapped to any other web async framework (eg: starlette or blacksheep), but ideally we should eval that once we have metrics on this.

Copy link
Copy Markdown
Member

@vgrozdanic vgrozdanic left a comment

Choose a reason for hiding this comment

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

Had a chat with Gio offline, approving to unblock him with testing of async proxy. This will not be run in a production, and it's more of a PoC than the real final thing.

We'll also explore the way to do this with Django and not rely on additional dependencies, but this will primarly be used to see if we can have an async proxy that will work end-to-end

@gi0baro gi0baro merged commit 057b1b4 into master May 19, 2026
85 checks passed
@gi0baro gi0baro deleted the gi0baro/apigateway-async-dedicated-package branch May 19, 2026 12:59
@markstory
Copy link
Copy Markdown
Member

I also don't like the addition of another web-framework. It adds complexity and redundancy that we should be avoiding given the scope and complexity of this application already.

It can be relatively easy swapped to any other web async framework (eg: starlette or blacksheep), but ideally we should eval that once we have metrics on this.

Do we really need another async runtime though? Django has asgi support, and I think the previous implementation had enough sync/async bridge points that we aren't really comparing similar solutions. Also if we're switching frameworks again, any metrics we collect will need to be re-done.

I'm also surprised to see this getting approved and merged without any automated tests ☹️

@vgrozdanic
Copy link
Copy Markdown
Member

Django has asgi support, and I think the previous implementation had enough sync/async bridge points that we aren't really comparing similar solutions

I agree, I talked about this with Gio as well, we can build a proper async proxy with Django as well

I'm also surprised to see this getting approved and merged without any automated tests

I see your point, and agree with you. From my talks with Gio, this will only be used to verify end-to-end that this is possible to do and that it will behave the way we expect it, so for me it was ok to let it live here as a temporary PoC in a separate app.

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.

4 participants