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

Add operationName variable for health_check_query #837

Closed
wants to merge 2 commits into from

Conversation

kutysam
Copy link

@kutysam kutysam commented Jun 25, 2021

Fixes #835

There is another issue that is slightly related #768 but the code is standalone for healthcheckquery, thus I am fixing this just for health check.

This is the payload for the data being sent by apollographql healthcheck and it is missing operationName
image

Why do we need this?

  1. If you do a call with graphiql, you can see a operationName key being sent out too.
  2. This operationName will be logged in metrics such as gql.operation.name in netflix dgs framework metrics https://netflix.github.io/dgs/advanced/instrumentation/#shared-tags
  3. For us to filter out health check requests, we can use operationName to filter them out.

These images show the payload within graphiql
image
image

@apollo-cla
Copy link

@kutysam: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@kutysam
Copy link
Author

kutysam commented Jun 25, 2021

I've signed the agreement..
The ci is failing but I'm sure it is not due to my commit.

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

I've suggested we stick to the exact operation name with the double underscore convention.
Otherwise, LGTM! Can you please add an entry to gateway-js/CHANGELOG.md?

@@ -119,6 +119,7 @@ export const GCS_RETRY_COUNT = 5;

export const HEALTH_CHECK_QUERY =
'query __ApolloServiceHealthCheck__ { __typename }';
export const HEALTH_CHECK_QUERY_OPERATION_NAME = 'ApolloServiceHealthCheck';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const HEALTH_CHECK_QUERY_OPERATION_NAME = 'ApolloServiceHealthCheck';
export const HEALTH_CHECK_QUERY_OPERATION_NAME = '__ApolloServiceHealthCheck__';

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

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

Is this really necessary? See my comment: #835 (comment)

If we suddenly have a pressing need to go above and beyond the standard GraphQL-over-HTTP protocol and always include operationName even when there's just one operation, then this would just be the tip of the iceberg. I'm sure there are hundreds of places across Apollo open source projects where we would need to make a similar change.

@glasser
Copy link
Member

glasser commented Jul 20, 2021

Will fix #835 in #870.

@glasser glasser closed this Jul 20, 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 this pull request may close these issues.

Missing operationName in __ApolloServiceHealthCheck__ query.
5 participants