Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
snigdha920 opened this issue Jul 18, 2024 · 2 comments
Open

Comments

@snigdha920
Copy link

snigdha920 commented Jul 18, 2024

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

  • OS: macOS
  • Bun: 1.1.13
  • @graphql-yoga/* version(s): 5.6.1

Additional context

No response

@snigdha920 snigdha920 changed the title @graphql-yoga/plugin-prometheus breaks subscriptions over graphql-ws Jul 18, 2024
@snigdha920 snigdha920 changed the title @graphql-yoga/plugin-prometheus breaks subscriptions over graphql-ws Jul 18, 2024
@snigdha920
Copy link
Author

snigdha920 commented Jul 18, 2024

The error is here:

paramsByRequest.set(request, {

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
Copy link
Author

snigdha920 commented Jul 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant