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

apollo-usage-report - get missing operationName from the OperationDefinitionNode #3488

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kroupacz
Copy link
Contributor

@kroupacz kroupacz commented Nov 12, 2024

Description

If someone send GraphQL query without operationName in request body, we should try to get it from the OperationDefinitionNode after the query parsing is finished.

I added this logic to the onParse (onParseEnd) handler, but I'm not sure if all the logic of the onEnveloped handler should be moved to the onParseEnd handler?

  • e.g. the operationName is missing if someone send request with body
{
  "query": "query TestQueryName{me{userAccount{id loginName}}}"
}

and the result in Apollo Studio metrics is: ❌
screenshot_1

  • if someone send request with body
{
  "query": "query TestQueryName{me{userAccount{id loginName}}}",
  "operationName": "TestQueryName"
}

the result in Apollo Studio metrics is: ✅
screenshot_2

Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: 896bea9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-yoga/plugin-apollo-usage-report Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kroupacz
Copy link
Contributor Author

@EmrysMyrddin or @ardatan 🙏

@@ -271,3 +297,14 @@ async function sendTrace(
logger.error('Failed to send trace:', err);
}
}

function isDocumentNode(data: unknown): data is DocumentNode {
const isObject = (data: unknown): data is Record<string, unknown> =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this has to be inlined into the isDocument function ? It's not using any variable from the scope.


function isDocumentNode(data: unknown): data is DocumentNode {
const isObject = (data: unknown): data is Record<string, unknown> =>
Object.prototype.toString.call(data).slice(8, -1) === 'Object';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to use this trick instead of typeof data === "object" ?

Copy link
Collaborator

@EmrysMyrddin EmrysMyrddin left a comment

Choose a reason for hiding this comment

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

I agree that deriving the operation name from the query document is a good idea.

If it's needed, I don't see anything the prevent us to move the whole logic to the onParse hook.

Because in the current implementation of the PR, the signature will be computed twice for each query without operation name, which can be expansive for huge queries.

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

Successfully merging this pull request may close these issues.

2 participants