Skip to content

Missing operationName in __ApolloServiceHealthCheck__ query. #835

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

Closed
kutysam opened this issue Jun 25, 2021 · 5 comments
Closed

Missing operationName in __ApolloServiceHealthCheck__ query. #835

kutysam opened this issue Jun 25, 2021 · 5 comments

Comments

@kutysam
Copy link

kutysam commented Jun 25, 2021

This is regarding https://github.com/apollographql/federation/blob/main/gateway-js/src/index.ts#L670

Apollo has a healthcheck query 'query ApolloServiceHealthCheck { __typename }'`
However, this query doesn't specify the operationName field within the JSON body.

In iGQL, when we send the same request, we do get a operationName: __ApolloServiceHealthCheck__ within the POST body, You guys can turn on developmental tools and take a look at the request if needed.

operationName is very helpful in certain libraries where the have metrics just for operationName. It will be good to have an operationName for this, as well as serviceSDL

@glasser
Copy link
Member

glasser commented Jun 25, 2021

Specifying operationName alongside your operation is only needed in the special case where your operation document contains multiple top-level documents. It's just for disambiguation. See eg the unofficial spec
for GraphQL over HTTP
:

operationName is only required if multiple operations are present in the query.

What tooling do you have that requires it to always be specified?

@ChristopheVandePoel
Copy link

@glasser I get what you mean (both here and in the raised Pull Request for this). Would it make more sense that there is a way to configure health check headers in stead? In the case of the introspection query, there is the option to configure this. For the health check, the only option right now is to parse the query (and really every query) and check if it is matching query __ApolloServiceHealthCheck__ { __typename }.

That would add overhead to every call, and it is fragile, because it is most likely considered Apollo Internals that can change. If the health-check function allowed to add a config to be included that sets the headers, it would solve both problems.

@kutysam
Copy link
Author

kutysam commented Jun 29, 2021

What tooling do you have that requires it to always be specified?
Tooling: https://github.com/netflix/dgs-framework/ Netflix DGS
It's not a requirement to let it be always specified, queries work with/ without it.
However, if we want to see metrics, they rely on the operationName. If operationName is not present, it will be anoynomous.
-> https://netflix.github.io/dgs/advanced/instrumentation/#shared-tags gql.operation.name

We are trying to differentiate user graphql queries and healthchecks within metrics and currently there is no way of doing it without the operationName flag. And if we do parse the body, as what Christophe mentioned, there is too much overhead if we check the body of every healthcheck.

@glasser
Copy link
Member

glasser commented Jun 29, 2021

I think it would be reasonable for us to provide some sort of HTTP header with health checks that make it clear that it's an HTTP header...

@glasser
Copy link
Member

glasser commented Jul 20, 2021

I"m going to solve this as part of #870.

  • gateway will provide a special value for a new option to GraphQLDataSource.process when it's a health check
  • this value will be passed on to willSendRequest as well in GraphQLRemoteDataSource.process
  • so you can make a GraphQLRemoteDataSource subclass that uses willSendRequest to set whatever headers you want when it's a health check

@glasser glasser closed this as completed in 50b62a7 Aug 3, 2021
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 a pull request may close this issue.

3 participants