[Bugfix] Refactor Metrics (#1280)

## Main Changes

* Empty HTTP request now returns 400
* Mapped ValidationError and SyntaxError to BAD_REQUEST for micrometer such that they reflect the proper status.

### Details

* Added "DgsException" for easier error handling
* Removed explicit empty query check so instrumentation can catch InvalidSyntax
* Re-introduced micrometer to the example project
* Added tests for DgsException
* Added DGS execution result + tests
* Teach HttpHandlers about dgsExecutionResult
* Updated instrumentation examples
* Added regression test for extension on response
* Fixed expected status code in empty query test
* Added backwards compat for old method of adding headers
* Fixed test expecting 200 when should be 400

graphql-dgs-example-java/build.gradle.kts

@@ -25,9 +25,11 @@ dependencies {
     // please review the Spring Boot Docs in the link below for additional information.
     // https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-metrics-export-simple
     implementation("org.springframework.boot:spring-boot-starter-actuator")
+
     // Adding the DGS Micrometer integration that provides timers for gql.*
     // For additional information go to the link below:
     // https://netflix.github.io/dgs/advanced/instrumentation/
+    implementation(project(":graphql-dgs-spring-boot-micrometer"))
 
     implementation("com.github.ben-manes.caffeine:caffeine")
     testImplementation(project(":graphql-dgs-client"))

graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java

@@ -16,30 +16,25 @@
 
 package com.netflix.graphql.dgs.example.datafetcher;
 
-import com.netflix.graphql.dgs.mvc.DgsRestController;
+import com.netflix.graphql.dgs.internal.DgsExecutionResult;
 import graphql.ExecutionResult;
-import graphql.ExecutionResultImpl;
 import graphql.execution.instrumentation.SimpleInstrumentation;
 import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
+import org.springframework.http.HttpHeaders;
 import org.springframework.stereotype.Component;
 
-import java.util.HashMap;
-import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 
 @Component
 public class MyInstrumentation extends SimpleInstrumentation {
     @Override
     public CompletableFuture<ExecutionResult> instrumentExecutionResult(ExecutionResult executionResult, InstrumentationExecutionParameters parameters) {
-        HashMap<Object, Object> extensions = new HashMap<>();
-        if(executionResult.getExtensions() != null) {
-            extensions.putAll(executionResult.getExtensions());
-        }
+        HttpHeaders responseHeaders = new HttpHeaders();
+        responseHeaders.add("myHeader", "hello");
 
-        Map<String, String> responseHeaders = new HashMap<>();
-        responseHeaders.put("myHeader", "hello");
-        extensions.put(DgsRestController.DGS_RESPONSE_HEADERS_KEY, responseHeaders);
-
-        return super.instrumentExecutionResult(new ExecutionResultImpl(executionResult.getData(), executionResult.getErrors(), extensions), parameters);
+        return super.instrumentExecutionResult(new DgsExecutionResult(
+                executionResult,
+                responseHeaders
+        ), parameters);
     }
 }

graphql-dgs-example-java/src/main/resources/application.yml

@@ -2,6 +2,13 @@ spring:
   application:
     name: dgs-example
 
+management:
+  endpoints:
+    web:
+      exposure:
+        include:
+          - metrics
+
 logging:
   level:
     com.netflix.graphql.dgs.example: DEBUG

graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt

@@ -6,6 +6,7 @@ import com.netflix.graphql.dgs.metrics.DgsMetrics.GqlMetric
 import com.netflix.graphql.dgs.metrics.DgsMetrics.GqlTag
 import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsGraphQLMetricsTagsProvider
 import com.netflix.graphql.dgs.metrics.micrometer.utils.QuerySignatureRepository
+import com.netflix.graphql.types.errors.ErrorType
 import graphql.ExecutionInput
 import graphql.ExecutionResult
 import graphql.GraphQLError
@@ -344,11 +345,11 @@ class DgsGraphQLMetricsInstrumentation(
                 when (error) {
                     is ValidationError -> {
                         errorPath = error.queryPath ?: emptyList()
-                        errorType = errorType(error)
+                        errorType = ErrorType.BAD_REQUEST.name
                     }
                     is InvalidSyntaxError -> {
                         errorPath = emptyList()
-                        errorType = errorType(error)
+                        errorType = ErrorType.BAD_REQUEST.name
                     }
                     else -> {
                         errorPath = error.path ?: emptyList()
@@ -368,10 +369,6 @@ class DgsGraphQLMetricsInstrumentation(
             return dedupeErrorPaths.values
         }
 
-        private fun <T : GraphQLError> errorType(error: T): String {
-            return error.errorType?.toString() ?: TagUtils.TAG_VALUE_UNKNOWN
-        }
-
         private fun <T : GraphQLError> errorTypeExtension(error: T): String {
             return extension(error, "errorType", TagUtils.TAG_VALUE_UNKNOWN)
         }

graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt

@@ -27,11 +27,13 @@ import com.netflix.graphql.dgs.DgsTypeDefinitionRegistry
 import com.netflix.graphql.dgs.InputArgument
 import com.netflix.graphql.dgs.exceptions.DefaultDataFetcherExceptionHandler
 import com.netflix.graphql.dgs.exceptions.DgsBadRequestException
+import com.netflix.graphql.dgs.exceptions.DgsException
 import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsContextualTagCustomizer
 import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsExecutionTagCustomizer
 import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsFieldFetchTagCustomizer
 import com.netflix.graphql.dgs.metrics.micrometer.utils.QuerySignatureRepository
 import com.netflix.graphql.types.errors.ErrorDetail
+import com.netflix.graphql.types.errors.ErrorType
 import com.netflix.graphql.types.errors.TypedGraphQLError
 import graphql.GraphQLError
 import graphql.execution.DataFetcherExceptionHandler
@@ -388,7 +390,7 @@ class MicrometerServletSmokeTest {
                     .and("gql.query.complexity", "none")
                     .and("gql.query.sig.hash", "none")
                     .and("gql.errorDetail", "none")
-                    .and("gql.errorCode", "InvalidSyntax")
+                    .and("gql.errorCode", "BAD_REQUEST")
                     .and("gql.path", "[]")
                     .and("outcome", "failure")
             )
@@ -448,7 +450,7 @@ class MicrometerServletSmokeTest {
                     .and("gql.query.complexity", "none")
                     .and("gql.query.sig.hash", "none")
                     .and("gql.errorDetail", "none")
-                    .and("gql.errorCode", "ValidationError")
+                    .and("gql.errorCode", "BAD_REQUEST")
                     .and("gql.path", "[hello]")
                     .and("outcome", "failure")
             )
@@ -479,8 +481,12 @@ class MicrometerServletSmokeTest {
                     """
                        |{
                        |    "errors":[
-                       |       {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[],
-                       |           "path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}}
+                       |      {
+                       |       "message":"java.lang.IllegalStateException: Exception triggered.",
+                       |       "locations": [],
+                       |       "path": ["triggerInternalFailure"],
+                       |       "extensions": {"errorType":"INTERNAL"}
+                        |     }
                        |    ],
                        |    "data":{"triggerInternalFailure":null}
                        |}
@@ -550,7 +556,7 @@ class MicrometerServletSmokeTest {
                     """
                         |{
                         |   "errors":[
-                        |      {"message":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException: Exception triggered.",
+                        |      {"message":"Exception triggered.",
                         |          "locations":[],"path":["triggerBadRequestFailure"],
                         |          "extensions":{"errorType":"BAD_REQUEST"}}
                         |   ],
@@ -688,18 +694,12 @@ class MicrometerServletSmokeTest {
             .andExpect(
                 content().json(
                     """
-                    |{
-                    |    "errors":[
-                    |        {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[],
-                    |            "path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}},
-                    |        {"message":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException: Exception triggered.","locations":[],
-                    |            "path":["triggerBadRequestFailure"],"extensions":{"errorType":"BAD_REQUEST"}},
-                    |        {"message":"Exception triggered.","locations":[],
-                    |            "path":["triggerCustomFailure"],
-                    |            "extensions":{"errorType":"UNAVAILABLE","errorDetail":"ENHANCE_YOUR_CALM"}}
-                    |    ],
-                    |    "data":{"triggerInternalFailure":null,"triggerBadRequestFailure":null,"triggerCustomFailure":null}
-                    |}
+                    | {"errors":[
+                    |    {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[],"path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}},
+                    |    {"message":"Exception triggered.","locations":[],"path":["triggerBadRequestFailure"],"extensions":{"class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException","errorType":"BAD_REQUEST"}},
+                    |    {"message":"Exception triggered.","locations":[],"path":["triggerCustomFailure"],"extensions":{"errorType":"UNAVAILABLE","errorDetail":"ENHANCE_YOUR_CALM"}}
+                    |  ],
+                    |  "data":{"triggerInternalFailure":null,"triggerBadRequestFailure":null,"triggerCustomFailure":null}}
                     """.trimMargin(),
                     false
                 )
@@ -969,5 +969,5 @@ class MicrometerServletSmokeTest {
         }
     }
 
-    class CustomException(message: String?) : java.lang.IllegalStateException(message)
+    class CustomException(message: String) : DgsException(message = message, errorType = ErrorType.INTERNAL)
 }

graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt

@@ -20,6 +20,7 @@ import com.fasterxml.jackson.core.JsonParseException
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.databind.exc.MismatchedInputException
 import com.fasterxml.jackson.module.kotlin.readValue
+import com.netflix.graphql.dgs.internal.DgsExecutionResult
 import com.netflix.graphql.dgs.reactive.DgsReactiveQueryExecutor
 import graphql.ExecutionResult
 import org.slf4j.Logger
@@ -74,9 +75,15 @@ class DefaultDgsWebfluxHttpHandler(
                 }
 
         return executionResult.flatMap { result ->
+            val dgsExecutionResult = when (result) {
+                is DgsExecutionResult -> result
+                else -> DgsExecutionResult(result)
+            }
+
             ServerResponse
-                .ok()
-                .bodyValue(result.toSpecification())
+                .status(dgsExecutionResult.status)
+                .headers { it.addAll(dgsExecutionResult.headers) }
+                .bodyValue(dgsExecutionResult.toSpecification())
         }.onErrorResume { ex ->
             when (ex) {
                 is JsonParseException ->

graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt

@@ -77,16 +77,19 @@ class MalformedQueryContentTest {
             .bodyValue("{ }")
             .exchange()
             .expectStatus()
-            .isOk
+            .isBadRequest
             .expectBody()
             .json(
                 """
                 {
                   "errors":[
                     {
-                      "message": "The query is null or empty.",
+                      "message": "GraphQL operations must contain a non-empty `query`.",
                       "locations":[],
-                      "extensions":{"errorType":"BAD_REQUEST"}
+                      "extensions": {
+                        "errorType":"BAD_REQUEST",
+                        "class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException"
+                      }
                     }
                   ]
                 }    

graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt

@@ -89,18 +89,21 @@ class MalformedQueryContentTest {
                 .content("{  }")
 
         mvc.perform(uriBuilder)
-            .andExpect(status().isOk)
+            .andExpect(status().isBadRequest)
             .andExpect(
                 content().json(
                     """
                     {
                       "errors":[
-                        {
-                          "message": "The query is null or empty.",
-                          "locations": [],
-                          "extensions": {"errorType":"BAD_REQUEST"}
-                        }
-                     ]
+                          {
+                            "message":"GraphQL operations must contain a non-empty `query`.",
+                            "locations":[],
+                            "extensions":{
+                              "class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException",
+                              "errorType":"BAD_REQUEST"
+                            }
+                          }
+                       ]
                     }
                     """.trimIndent()
                 )

graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt

@@ -18,15 +18,13 @@ package com.netflix.graphql.dgs.mvc
 
 import com.fasterxml.jackson.core.JsonParseException
 import com.fasterxml.jackson.databind.ObjectMapper
-import com.fasterxml.jackson.databind.exc.InvalidDefinitionException
 import com.fasterxml.jackson.databind.exc.MismatchedInputException
 import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
 import com.fasterxml.jackson.module.kotlin.readValue
 import com.netflix.graphql.dgs.DgsQueryExecutor
+import com.netflix.graphql.dgs.internal.DgsExecutionResult
 import com.netflix.graphql.dgs.internal.utils.MultipartVariableMapper
 import com.netflix.graphql.dgs.internal.utils.TimeTracer
-import graphql.ExecutionResultImpl
-import graphql.GraphqlErrorBuilder
 import graphql.execution.reactive.SubscriptionPublisher
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
@@ -75,7 +73,9 @@ open class DgsRestController(
 ) {
 
     companion object {
-        const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers"
+        // defined in here and DgsExecutionResult, for backwards compatibility.
+        // keep these two variables synced.
+        const val DGS_RESPONSE_HEADERS_KEY = DgsExecutionResult.DGS_RESPONSE_HEADERS_KEY
         private val logger: Logger = LoggerFactory.getLogger(DgsRestController::class.java)
     }
 
@@ -226,40 +226,9 @@ open class DgsRestController(
                 .body("Trying to execute subscription on /graphql. Use /subscriptions instead!")
         }
 
-        val responseHeaders = if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) {
-            val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY]
-            val responseHeaders = HttpHeaders()
-            if (dgsResponseHeaders is Map<*, *>) {
-                dgsResponseHeaders.forEach {
-                    if (it.key != null) {
-                        responseHeaders.add(it.key.toString(), it.value?.toString())
-                    }
-                }
-            } else {
-                logger.warn(
-                    "{} must be of type java.util.Map, but was {}",
-                    DGS_RESPONSE_HEADERS_KEY,
-                    dgsResponseHeaders?.javaClass?.name
-                )
-            }
-
-            executionResult.extensions.remove(DGS_RESPONSE_HEADERS_KEY)
-            responseHeaders
-        } else HttpHeaders()
-
-        val result = try {
-            TimeTracer.logTime(
-                { mapper.writeValueAsBytes(executionResult.toSpecification()) },
-                logger,
-                "Serialized JSON result in {}ms"
-            )
-        } catch (ex: InvalidDefinitionException) {
-            val errorMessage = "Error serializing response: ${ex.message}"
-            val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build())
-            logger.error(errorMessage, ex)
-            mapper.writeValueAsBytes(errorResponse.toSpecification())
+        return when (executionResult) {
+            is DgsExecutionResult -> executionResult.toSpringResponse()
+            else -> DgsExecutionResult(executionResult).toSpringResponse()
         }
-
-        return ResponseEntity(result, responseHeaders, HttpStatus.OK)
     }
 }

graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt

@@ -203,9 +203,22 @@ class DgsRestControllerTest {
 
         val result = DgsRestController(dgsQueryExecutor).graphql(requestBody.toByteArray(), null, null, null, httpHeaders, webRequest)
         assertThat(result.headers["myHeader"]).contains("hello")
+
+        assertThat(result.body).isInstanceOfSatisfying(ByteArray::class.java) { body ->
+            val mapper = jacksonObjectMapper()
+            val (_, _, extensions) = mapper.readValue(body, GraphQLResponse::class.java)
+            assertThat(extensions).isNull()
+        }
     }
 
-    data class GraphQLResponse(val data: Map<String, Any> = emptyMap(), val errors: List<GraphQLError> = emptyList())
+    data class GraphQLResponse(
+        val data: Map<String, Any> = emptyMap(),
+        val errors: List<GraphQLError> = emptyList(),
+
+        // should not be on the response; only here so we can
+        // test that it does not exist.
+        val extensions: Map<String, Any>?
+    )
 
     @JsonIgnoreProperties(ignoreUnknown = true)
     data class GraphQLError(val message: String)

graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt

@@ -46,23 +46,17 @@ class DefaultDataFetcherExceptionHandler : DataFetcherExceptionHandler {
             "Exception while executing data fetcher for ${handlerParameters.path}: ${exception.message}",
             exception
         )
-        val graphqlError = if (springSecurityAvailable && isSpringSecurityAccessException(exception)) {
-            TypedGraphQLError.newPermissionDeniedBuilder()
-                .message("%s: %s", exception::class.java.name, exception.message)
-                .path(handlerParameters.path).build()
-        } else if (exception is DgsEntityNotFoundException) {
-            TypedGraphQLError.newNotFoundBuilder()
-                .message("%s: %s", exception::class.java.name, exception.message)
-                .path(handlerParameters.path).build()
-        } else if (exception is DgsBadRequestException) {
-            TypedGraphQLError.newBadRequestBuilder()
-                .message("%s: %s", exception::class.java.name, exception.message)
-                .path(handlerParameters.path).build()
-        } else {
-            TypedGraphQLError.newInternalErrorBuilder()
-                .message("%s: %s", exception::class.java.name, exception.message)
-                .path(handlerParameters.path).build()
+
+        val graphqlError = when (exception) {
+            is DgsException -> exception.toGraphQlError(handlerParameters.path)
+            else -> when {
+                springSecurityAvailable && isSpringSecurityAccessException(exception) -> TypedGraphQLError.newPermissionDeniedBuilder()
+                else -> TypedGraphQLError.newInternalErrorBuilder()
+            }.message("%s: %s", exception::class.java.name, exception.message)
+                .path(handlerParameters.path)
+                .build()
         }
+
         return DataFetcherExceptionHandlerResult.newResult()
             .error(graphqlError)
             .build()