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

Implement calculation of query complexity metric. #300

Merged
merged 1 commit into from Apr 28, 2021

Conversation

srinivasankavitha
Copy link
Contributor

@srinivasankavitha srinivasankavitha commented Apr 28, 2021

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

Query Complexity is the total number of nodes in the query. graphql-java provides a MaxQueryComplexityInstrumentation class that users can add and set limits on the query complexity (https://www.javadoc.io/doc/com.graphql-java/graphql-java/2019-07-13T23-07-14-8c71e18/graphql/analysis/MaxQueryComplexityInstrumentation.html)

This PR implements the query complexity calculation and sets the tag gql.queryComplexity with the value on the gql.query metric. To do so, we override the beginValidation() in the instrumentation class and implement the complexity by traversing the nodes using the same implementation as linked below:
Reference Implementation : https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java

In addition, we add the result of the query complexity calculation to our execution state for tagging as a metric.

Note that this does not provide any ability to limit the complexity of the query (as intended by MaxQueryComplexityInstrumentation). This implementation is simply for the framework to track the complexity for metrics.

Alternatives considered

I looked into using the MaxQueryComplexityFunction (provided as lambda) to extract the result of the query complexity computation, but this does not allow us to set the result in any execution specific state. This would have ideally avoided re-implementing beginValidation(). Since we don't have access to any execution specific state, I resorted to the approach above.

@@ -19,4 +19,6 @@ dependencies {
implementation(project(":graphql-dgs-subscriptions-websockets-autoconfigure"))
implementation("org.springframework.boot:spring-boot-starter-web")
implementation("io.projectreactor:reactor-core")
implementation(project(":graphql-dgs-spring-boot-micrometer"))
Copy link
Contributor

Choose a reason for hiding this comment

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

you will not this anymore as part of the pr. I just merged #297.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR removed micrometer from the example project - I bet there was a merge on #297 and then @srinivasankavitha responded to this comment :). I am working on #1058, I can re-add micrometer to this as part of that PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound good! Thanks, that would be helpful!

* Port the implementation from MaxQueryComplexityInstrumentation in graphql-java and store the computed complexity
* in the MetricsInstrumentationState for access to add tags to metrics.
*/
override fun beginValidation(parameters: InstrumentationValidationParameters): InstrumentationContext<List<ValidationError>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we send a PR to the graphql-java folks such that they expose this as part of a utility such that it allows some sort of composability that we can leverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that makes sense.

@srinivasankavitha srinivasankavitha merged commit febd26d into master Apr 28, 2021
@berngp berngp deleted the add-query-complexity branch October 20, 2022 01:48
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.

None yet

3 participants