Skip to content

pq: include operation name in "PQ not found" error #7768

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

glasser
Copy link
Member

@glasser glasser commented Jun 25, 2025

We have found that during migrations to PQ-based safelisting, it is helpful to have a bit more information about what operation the client is trying to run than just the hashed PQ ID, to help find the client app that is being blocked. While the client name can be found in the context, the operation name (if provided explicitly in the body's operationName field by the client) is helpful. We now provide this as an extension on the error.

(It is both directly helpful in that it is available in the JSON error response if you're looking at individual failures, and it also makes it easy for a router_service-level Rust or Rhai plugin that's trying to provide more observability into PQ errors to look at the error for the extra data. This error occurs before supergraph_service runs, which means that the operation name is not in an easily accessible place for plugins.)


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • PR description explains the motivation for the change and relevant context for reviewing
  • PR description links appropriate GitHub/Jira tickets (creating when necessary)
  • Changeset is included for user-facing changes
  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Metrics and logs are added3 and documented
  • Tests added and passing4
    • Unit tests
    • Integration tests
    • Manual tests, as necessary

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices.

  4. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

This comment has been minimized.

@glasser glasser force-pushed the glasser/pq-error-include-extensions branch from 8dc6ac4 to 95cdae4 Compare June 25, 2025 16:01
@apollo-librarian
Copy link

apollo-librarian bot commented Jun 25, 2025

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 1 changed, 0 removed
* (developer-tools)/apollo-mcp-server/(latest)/best-practices.mdx

Build ID: 548084269000d84f260ab156

URL: https://www.apollographql.com/docs/deploy-preview/548084269000d84f260ab156

@glasser glasser marked this pull request as ready for review June 25, 2025 16:01
@glasser glasser requested review from a team as code owners June 25, 2025 16:01
@glasser glasser force-pushed the glasser/pq-error-include-extensions branch from 95cdae4 to 47dbcc9 Compare June 25, 2025 16:16
@glasser glasser changed the title pq: include operation name and client name in "PQ not found" error pq: include operation name in "PQ not found" error Jun 25, 2025
@glasser glasser force-pushed the glasser/pq-error-include-extensions branch from 47dbcc9 to 121067c Compare June 25, 2025 16:23
@glasser
Copy link
Member Author

glasser commented Jun 25, 2025

This is in some sense a workaround for #3642 (though not just for Rhai), but I think it is likely to be helpful in other circumstances. Another more plugin-focused design could be to add this to the context instead of the error, though then we'd have to choose a name that indicates "this is the explicit operation name found in the request" as opposed to the existing context entry (added later by query analysis) that means "this is the resolved operation name we are using".

We have found that during migrations to PQ-based safelisting, it is
helpful to have a bit more information about what operation the client
is trying to run than just the hashed PQ ID, to help find the client app
that is being blocked. While the client name can be found in the
context, the operation name (if provided explicitly in the body's
`operationName` field by the client) is helpful. We now provide this as
an extension on the error.

(It is both directly helpful in that it is available in the JSON error
response if you're looking at individual failures, and it also makes it
easy for a router_service-level Rust or Rhai plugin that's trying to
provide more observability into PQ errors to look at the error for the
extra data. This error occurs before supergraph_service runs, which
means that the operation name is not in an easily accessible place for
plugins.)
@glasser glasser force-pushed the glasser/pq-error-include-extensions branch from 121067c to ecf5727 Compare June 25, 2025 16:50
@glasser glasser merged commit 1b9467b into dev Jun 25, 2025
14 of 15 checks passed
@glasser glasser deleted the glasser/pq-error-include-extensions branch June 25, 2025 18:04
@abernix abernix mentioned this pull request Jul 1, 2025
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