From 25e63a9cf6e485b3f4d169ea3afe62914e6287f2 Mon Sep 17 00:00:00 2001 From: Jessie Jacobs Date: Wed, 21 May 2025 14:39:39 -0700 Subject: [PATCH 1/2] Look for errors in the graphql response for metric tagging --- .../DgsGraphQLMetricsInstrumentation.kt | 14 ++- .../micrometer/MicrometerServletSmokeTest.kt | 115 ++++++++++++++++++ 2 files changed, 125 insertions(+), 4 deletions(-) diff --git a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt index 2b4787bc7..24d0dde4d 100644 --- a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt +++ b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt @@ -10,9 +10,11 @@ import com.netflix.graphql.types.errors.ErrorType import graphql.ExecutionInput import graphql.ExecutionResult import graphql.GraphQLError +import graphql.GraphQLException import graphql.InvalidSyntaxError import graphql.analysis.FieldComplexityCalculator import graphql.analysis.QueryComplexityCalculator +import graphql.execution.DataFetcherResult import graphql.execution.ExecutionContext import graphql.execution.instrumentation.InstrumentationContext import graphql.execution.instrumentation.InstrumentationState @@ -154,18 +156,18 @@ class DgsGraphQLMetricsInstrumentation( try { val result = dataFetcher.get(environment) if (result is CompletionStage<*>) { - result.whenComplete { _, error -> + result.whenComplete { value, error -> recordDataFetcherMetrics( registry, sampler, state, parameters, - error, - baseTags, + checkResponseForErrors(value, error), + baseTags ) } } else { - recordDataFetcherMetrics(registry, sampler, state, parameters, null, baseTags) + recordDataFetcherMetrics(registry, sampler, state, parameters, checkResponseForErrors(result, null), baseTags) } result } catch (exc: Exception) { @@ -175,6 +177,10 @@ class DgsGraphQLMetricsInstrumentation( } } + private fun checkResponseForErrors(value: Any?, error: Throwable?): Throwable? = error + ?: (value as? DataFetcherResult<*>)?.takeIf { it.hasErrors() } + ?.let { GraphQLException("GraphQL errors in response: ${it.errors}") } + /** * Port the implementation from MaxQueryComplexityInstrumentation in graphql-java and store the computed complexity * in the MetricsInstrumentationState for access to add tags to metrics. diff --git a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt index 3a40a6d73..b0e322f3b 100644 --- a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt +++ b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt @@ -39,6 +39,7 @@ import graphql.GraphQLError import graphql.execution.DataFetcherExceptionHandler import graphql.execution.DataFetcherExceptionHandlerParameters import graphql.execution.DataFetcherExceptionHandlerResult +import graphql.execution.DataFetcherResult import graphql.schema.DataFetchingEnvironment import graphql.schema.idl.SchemaParser import graphql.schema.idl.TypeDefinitionRegistry @@ -561,6 +562,106 @@ class MicrometerServletSmokeTest { ) } + @Test + fun `Assert metrics for a successful async response with errors`() { + mvc + .perform( + MockMvcRequestBuilders + .post("/graphql") + .contentType(MediaType.APPLICATION_JSON) + .content("""{ "query": "{triggerSuccessfulRequestWithErrorAsync}" }"""), + ).andExpect(status().isOk) + .andExpect( + content().json( + """ + |{ + | "errors":[ + | {"message":"Exception triggered."} + | ], + | "data":{"triggerSuccessfulRequestWithErrorAsync":"Some data..."} + |} + """.trimMargin(), + false, + ), + ) + + val meters = fetchMeters("gql.") + + assertThat(meters).containsOnlyKeys("gql.error", "gql.query", "gql.resolver") + + assertThat(meters["gql.error"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat((meters["gql.error"]?.first() as CumulativeCounter).count()).isEqualTo(1.0) + assertThat(meters["gql.error"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + + assertThat(meters["gql.query"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat(meters["gql.query"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + + assertThat(meters["gql.resolver"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat(meters["gql.resolver"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + } + + @Test + fun `Assert metrics for a successful sync response with errors`() { + mvc + .perform( + MockMvcRequestBuilders + .post("/graphql") + .contentType(MediaType.APPLICATION_JSON) + .content("""{ "query": "{triggerSuccessfulRequestWithErrorSync}" }"""), + ).andExpect(status().isOk) + .andExpect( + content().json( + """ + |{ + | "errors":[ + | {"message":"Exception triggered."} + | ], + | "data":{"triggerSuccessfulRequestWithErrorSync":"Some data..."} + |} + """.trimMargin(), + false, + ), + ) + + val meters = fetchMeters("gql.") + + assertThat(meters).containsOnlyKeys("gql.error", "gql.query", "gql.resolver") + + assertThat(meters["gql.error"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat((meters["gql.error"]?.first() as CumulativeCounter).count()).isEqualTo(1.0) + assertThat(meters["gql.error"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + + assertThat(meters["gql.query"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat(meters["gql.query"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + + assertThat(meters["gql.resolver"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) + assertThat(meters["gql.resolver"]?.first()?.id?.tags) + .containsAll( + Tags + .of("outcome", "failure") + ) + } + @Test fun `Assert metrics for custom error`() { mvc @@ -793,6 +894,8 @@ class MicrometerServletSmokeTest { | triggerInternalFailure: String | triggerBadRequestFailure:String | triggerCustomFailure: String + | triggerSuccessfulRequestWithErrorAsync:String + | triggerSuccessfulRequestWithErrorSync:String |} | |type Mutation{ @@ -835,6 +938,18 @@ class MicrometerServletSmokeTest { @DgsQuery fun triggerBadRequestFailure(): String = throw DgsBadRequestException("Exception triggered.") + @DgsQuery + fun triggerSuccessfulRequestWithErrorAsync(): CompletableFuture> = CompletableFuture.supplyAsync { DataFetcherResult.newResult() + .data("Some data...") + .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) + .build()} + + @DgsQuery + fun triggerSuccessfulRequestWithErrorSync(): DataFetcherResult = DataFetcherResult.newResult() + .data("Some data...") + .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) + .build() + @DgsQuery fun triggerCustomFailure(): String = throw CustomException("Exception triggered.") From 0c345b64478cd8233d4b17092d02aa13a06bae37 Mon Sep 17 00:00:00 2001 From: Jessie Jacobs Date: Wed, 21 May 2025 17:27:48 -0700 Subject: [PATCH 2/2] linting --- .../DgsGraphQLMetricsInstrumentation.kt | 13 ++++--- .../micrometer/MicrometerServletSmokeTest.kt | 34 +++++++++++-------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt index cefa43830..a96f86df5 100644 --- a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt +++ b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt @@ -181,7 +181,7 @@ class DgsGraphQLMetricsInstrumentation( state, parameters, checkResponseForErrors(value, error), - baseTags + baseTags, ) } } else { @@ -195,9 +195,14 @@ class DgsGraphQLMetricsInstrumentation( } } - private fun checkResponseForErrors(value: Any?, error: Throwable?): Throwable? = error - ?: (value as? DataFetcherResult<*>)?.takeIf { it.hasErrors() } - ?.let { GraphQLException("GraphQL errors in response: ${it.errors}") } + private fun checkResponseForErrors( + value: Any?, + error: Throwable?, + ): Throwable? = + error + ?: (value as? DataFetcherResult<*>) + ?.takeIf { it.hasErrors() } + ?.let { GraphQLException("GraphQL errors in response: ${it.errors}") } /** * Port the implementation from MaxQueryComplexityInstrumentation in graphql-java and store the computed complexity diff --git a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt index f06923653..491894312 100644 --- a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt +++ b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt @@ -711,21 +711,21 @@ class MicrometerServletSmokeTest { assertThat(meters["gql.error"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) assertThat(meters["gql.query"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) assertThat(meters["gql.query"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) assertThat(meters["gql.resolver"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) assertThat(meters["gql.resolver"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) } @@ -761,21 +761,21 @@ class MicrometerServletSmokeTest { assertThat(meters["gql.error"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) assertThat(meters["gql.query"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) assertThat(meters["gql.query"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) assertThat(meters["gql.resolver"]).isNotNull.hasSizeGreaterThanOrEqualTo(1) assertThat(meters["gql.resolver"]?.first()?.id?.tags) .containsAll( Tags - .of("outcome", "failure") + .of("outcome", "failure"), ) } @@ -1060,16 +1060,22 @@ class MicrometerServletSmokeTest { fun triggerBadRequestFailure(): String = throw DgsBadRequestException("Exception triggered.") @DgsQuery - fun triggerSuccessfulRequestWithErrorAsync(): CompletableFuture> = CompletableFuture.supplyAsync { DataFetcherResult.newResult() - .data("Some data...") - .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) - .build()} + fun triggerSuccessfulRequestWithErrorAsync(): CompletableFuture> = + CompletableFuture.supplyAsync { + DataFetcherResult + .newResult() + .data("Some data...") + .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) + .build() + } @DgsQuery - fun triggerSuccessfulRequestWithErrorSync(): DataFetcherResult = DataFetcherResult.newResult() - .data("Some data...") - .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) - .build() + fun triggerSuccessfulRequestWithErrorSync(): DataFetcherResult = + DataFetcherResult + .newResult() + .data("Some data...") + .error(TypedGraphQLError("Exception triggered.", null, null, null, null)) + .build() @DgsQuery fun triggerCustomFailure(): String = throw CustomException("Exception triggered.")