dotansimha / graphql-yoga

🧘 Rewrite of a fully-featured GraphQL Server with focus on easy setup, performance & great developer experience. The core of Yoga implements WHATWG Fetch API and can run/deploy on any JS environment.
https://the-guild.dev/graphql/yoga-server
MIT License
8.2k stars 566 forks source link

@graphql-yoga/plugin-prometheus breaks subscriptions over graphql-ws #3374

Open snigdha920 opened 1 month ago

snigdha920 commented 1 month ago

Describe the bug

On installing the prometheus plugin, subscriptions fail with the error:

65 |             return undefined;
66 |         },
67 |         onParse() {
68 |             return ({ result: document, context: { params, request } }) => {
69 |                 const operationAST = getOperationAST(document, params.operationName);
70 |                 paramsByRequest.set(request, {
                                     ^
TypeError: WeakMap keys must be objects or non-registered symbols
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/@graphql-yoga/plugin-prometheus/esm/index.js:70:33
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/@envelop/core/esm/orchestrator.js:111:17
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/src/index.ts:71:17
      at onSubscribe (/Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/src/index.ts:60:23)
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/graphql-ws/lib/server.mjs:146:124
      at onMessage (/Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/graphql-ws/lib/server.mjs:51:61)

If we comment out the plugin, subscriptions work again.

If we use the plugins from @envelop/prometheus, the subscriptions work again, but http://localhost:4000/metrics gives a 404.

What is the correct way to use the plugin with graphql-ws subscriptions?

I can also open a PR but will need guidance on what the fix is.

Your Example Website or App

https://github.com/snigdha920/bun-graphql-yoga-prometheus/tree/main

Steps to Reproduce the Bug or Issue

  1. Run bun dev
  2. Go to http://localhost:4000/graphql
  3. Run the subscription:
subscription MySubscription {
  dynamicLoading(loadTime: 10)
}
  1. You will see there is no data loading:
image

and this error in the console:

65 |             return undefined;
66 |         },
67 |         onParse() {
68 |             return ({ result: document, context: { params, request } }) => {
69 |                 const operationAST = getOperationAST(document, params.operationName);
70 |                 paramsByRequest.set(request, {
                                     ^
TypeError: WeakMap keys must be objects or non-registered symbols
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/@graphql-yoga/plugin-prometheus/esm/index.js:70:33
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/@envelop/core/esm/orchestrator.js:111:17
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/src/index.ts:71:17
      at onSubscribe (/Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/src/index.ts:60:23)
      at /Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/graphql-ws/lib/server.mjs:146:124
      at onMessage (/Users/snigdhasingh/Development/bun-graphql-yoga-prometheus/node_modules/graphql-ws/lib/server.mjs:51:61)
  1. Comment out the usePrometheus plugin, run the subscription again, works as expected
image

Expected behavior

I expected the subscription to return data

Screenshots or Videos

No response

Platform

Additional context

No response

snigdha920 commented 1 month ago

The error is here: https://github.com/dotansimha/graphql-yoga/blob/855600cd6f39102f8bdfd5d9f54bf62c9a26bc0f/packages/plugins/prometheus/src/index.ts#L107

When using subscriptions there is no request in the context, and we still try to use it as a key for setting params, adding something like:

// When parsing subscriptions, there is no request
if (!request) {
    return;
}
paramsByRequest.set(request, {
    document,
    operationName: operationAST?.name?.value,
    operationType: operationAST?.operation
});

fixes the issue.

But I'm also not sure why we're using request as a key, in the original @envelop/prometheus they use the entire context, which already takes care of this condition:

https://github.com/n1ru4l/envelop/blob/737f250a32517560d813ed479d671e1412822ec6/packages/plugins/prometheus/src/index.ts#L270

So perhaps the best fix is to use the context as the key?

snigdha920 commented 1 month ago

Right now I'm using the custom minimal metrics plugin, because I only really need the /metrics endpoint: https://github.com/snigdha920/bun-graphql-yoga-prometheus/tree/custom-plugin