Repository navigation
Meter and limit GraphQL subscriptions - #391
collins-uzu wants to merge 1 commit into
Conversation
|
@collins-uzu Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Needs a rebase — Worth knowing before you rebase: #402 replaced every Push the rebase and I will pick it up; nothing is queued ahead of it. |
|
Correction to my earlier review. I described this branch as hundreds of commits behind with tens of thousands of lines of deletions. The deletion figure was wrong — it came from Measured properly against current So the real cost is resolving 3 conflicts, not reconstructing the repository. I apologise for the framing. |
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean, and merges with only three conflicts — far fewer than my earlier review implied, and I have corrected that separately. The feature is one I want: an unmetered, unlimited subscription is the cheapest way for one API key to pin a process, and active_subscriptions on main only counts them.
The blocker is that src/api/graphql.ts has been rewritten underneath you, and the two versions disagree about how a subscription is delivered. This is a port, not a merge, which is why I am handing it back rather than resolving it myself.
What changed on main
Your branch subscribes to one topic and filters per subscriber:
subscribe: withFilter<…>(
async (_root, _args, { pubsub, apiKey }) => { … pubsub.subscribe(PRICE_TOPIC) … },
(payload, { pair, network }) => payload.priceUpdated.pair === pair && …
)main now routes by topic instead, so no per-subscriber filter callback runs at all:
subscribe: withScope('subscribe', (_root, { pair, network }, { pubsub }) => {
const topics = network == null
? KNOWN_NETWORKS.map(n => priceTopic(pair, n))
: [priceTopic(pair, network)]
…
queue.close.push(release) // release rides Mercurius's close list
})Two consequences for your code:
withFilteris gone, and with it the filter predicate. Routing happens by topic name now.withScopewraps it — that is #623's per-field scope enforcement, which landed this morning. Subscriptions are gated on thesubscribescope, so whatever you add has to sit inside that wrapper rather than replacing it.
What to port, and where it goes
Your three pieces all still apply; they just attach to different places:
- The per-key concurrent limit.
activeSubscriptionsByKey, themaxSubscriptionsPerKey()check and the increment belong at the top of the new resolver body, beforepubsub.subscribe(topics). Note the new signature destructures{ pubsub }only — you will needapiKeyfrom the context too. - The release path. Do not reimplement it.
mainalready has arelease()that ridesqueue.close, which is the part your version could not do — yourfinallyin the async generator fires when the generator is drained, not when the client disconnects. Add your per-key decrement into that existingrelease, so the count follows the real subscription lifetime. - The quota check.
checkQuota/recordUsage/parseCents(GRAPHQL_REQUEST_PRICE)go before the subscribe call, with the release called on the failure path — the existing.catchalready does that for you.
That last point is the one worth the most: main's release-on-close is strictly better than what your branch has, so porting onto it makes your limit more accurate rather than less.
Two smaller conflicts, both trivial: .env.example and README.md are additive on both sides and want a union.
Rebase, port those three pieces, and re-request — main is green (tsc clean, 1,874 tests), so anything that fails afterwards is yours. Happy to look quickly once it builds.
Summary
GRAPHQL_MAX_SUBSCRIPTIONS_PER_KEY(default: 10).Tests
npm test(62 files passed, 1 skipped; 643 tests passed, 1 skipped)npx tsc --noEmitCloses #363