Skip to content

Commit

Permalink
Merge pull request #519 from Netflix/feature/fix-operation-tag
Browse files Browse the repository at this point in the history 
Address incorrect operation tag value
  • Loading branch information
@berngp
berngp committed Aug 7, 2021
2 parents d313f45 + 6dddeba commit b48f832
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 38 deletions.
51 changes: 21 additions & 30 deletions ...ain/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ import graphql.analysis.QueryVisitorStub
import graphql.execution.instrumentation.*
import graphql.execution.instrumentation.SimpleInstrumentationContext.whenCompleted
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters
import graphql.language.Document
import graphql.language.OperationDefinition
import graphql.schema.DataFetcher
import graphql.schema.GraphQLNonNull
import graphql.schema.GraphQLObjectType
Expand All @@ -29,8 +28,7 @@ import io.micrometer.core.instrument.Timer
import org.slf4j.Logger
import org.slf4j.LoggerFactory
import java.util.*
import java.util.Optional.empty
import java.util.Optional.ofNullable
import java.util.Optional.*
import java.util.concurrent.CompletableFuture
import java.util.concurrent.CompletionStage

Expand Down Expand Up @@ -58,7 +56,6 @@ class DgsGraphQLMetricsInstrumentation(
state.startTimer()

state.operationName = ofNullable(parameters.operation)
state.operation = ofNullable(parameters.schema.queryType.name)

return object : SimpleInstrumentationContext<ExecutionResult>() {

Expand All @@ -78,13 +75,6 @@ class DgsGraphQLMetricsInstrumentation(
}
}

override fun instrumentDocumentAndVariables(
documentAndVariables: DocumentAndVariables,
parameters: InstrumentationExecutionParameters
): DocumentAndVariables {
return documentAndVariables
}

override fun instrumentExecutionResult(
executionResult: ExecutionResult,
parameters: InstrumentationExecutionParameters
Expand Down Expand Up @@ -164,18 +154,32 @@ class DgsGraphQLMetricsInstrumentation(
return@whenCompleted
}
val state: MetricsInstrumentationState = parameters.getInstrumentationState()
// compute fields that require a document
if (parameters.document != null) {
state.operationName =
if (state.operationName.isPresent) state.operationName
else TagUtils.resolveOperationNameFromUniqueOperationDefinition(parameters.document)
state.querySignature = optQuerySignatureRepository.flatMap { it.get(parameters.document, parameters) }
}
// compute the query complexity.
state.queryComplexity = ComplexityUtils.resolveComplexity(parameters)
}
}

override fun beginExecutionStrategy(
parameters: InstrumentationExecutionStrategyParameters
): ExecutionStrategyInstrumentationContext {
val state: MetricsInstrumentationState = parameters.getInstrumentationState()
if (parameters.executionContext.getRoot<Any>() == null) {
state.operation = of(parameters.executionContext.operationDefinition.operation.name.toUpperCase())
if (!state.operationName.isPresent) {
state.operationName = ofNullable(parameters.executionContext.operationDefinition?.name)
}
}
return object : ExecutionStrategyInstrumentationContext {
override fun onDispatched(result: CompletableFuture<ExecutionResult>) {
}

override fun onCompleted(result: ExecutionResult, t: Throwable) {
}
}
}

private fun recordDataFetcherMetrics(
registry: MeterRegistry,
timerSampler: Timer.Sample,
Expand Down Expand Up @@ -296,19 +300,6 @@ class DgsGraphQLMetricsInstrumentation(
const val TAG_VALUE_NONE = "none"
const val TAG_VALUE_UNKNOWN = "unknown"

fun resolveOperationNameFromUniqueOperationDefinition(document: Document): Optional<String> {
return if (document.definitions.isNotEmpty()) {
val operationDefinitions = document.definitions.filterIsInstance<OperationDefinition>()
if (operationDefinitions.size == 1) {
ofNullable(operationDefinitions.first().name)
} else {
empty()
}
} else {
empty()
}
}

fun resolveDataFetcherTagValue(parameters: InstrumentationFieldFetchParameters): String {
val type = parameters.executionStepInfo.parent.type
val parentType = if (type is GraphQLNonNull) {
Expand Down
66 changes: 58 additions & 8 deletions .../src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class MicrometerServletSmokeTest {
}

@Test
fun `Metrics for a successful request, implicit operation name`() {
fun `Metrics for a successful query`() {
mvc.perform(
// Note that the query below uses an aliased field, aliasing `ping` to `op_name`.
// We will also assert that the tag reflected by the metric is not affected by the alias.
Expand Down Expand Up @@ -136,6 +136,47 @@ class MicrometerServletSmokeTest {
)
}

@Test
fun `Metrics for a successful mutation`() {
mvc.perform(
// Note that the query below uses an aliased field, aliasing `ping` to `op_name`.
// We will also assert that the tag reflected by the metric is not affected by the alias.
MockMvcRequestBuilders
.post("/graphql")
.content("""{ "query": " mutation my_op_1{buzz}" }""".trimMargin())
).andExpect(status().isOk)
.andExpect(content().json("""{"data":{"buzz":"buzz"}}""", false))

val meters = fetchMeters()

assertThat(meters).containsOnlyKeys("gql.query", "gql.resolver")

assertThat(meters["gql.query"]).isNotNull.hasSize(1)
assertThat(meters["gql.query"]?.first()?.id?.tags)
.containsAll(
Tags.of("execution-tag", "foo")
.and("contextual-tag", "foo")
.and("outcome", "success")
.and("gql.operation", "MUTATION")
.and("gql.operation.name", "my_op_1")
.and("gql.query.complexity", "5")
.and("gql.query.sig.hash", MOCKED_QUERY_SIGNATURE.hash)
)

assertThat(meters["gql.resolver"]).isNotNull.hasSize(1)
assertThat(meters["gql.resolver"]?.first()?.id?.tags)
.containsAll(
Tags.of("field-fetch-tag", "foo")
.and("contextual-tag", "foo")
.and("gql.field", "Mutation.buzz")
.and("outcome", "success")
.and("gql.operation", "MUTATION")
.and("gql.operation.name", "my_op_1")
.and("gql.query.complexity", "5")
.and("gql.query.sig.hash", MOCKED_QUERY_SIGNATURE.hash)
)
}

@Test
fun `Metrics for a successful request with explicit operation name`() {
mvc.perform(
Expand All @@ -146,8 +187,8 @@ class MicrometerServletSmokeTest {
.content(
"""
| {
| "query": "query my_op_1{ping}",
| "operationName": "my_op_1"
| "query": "mutation my_m_1{buzz} query my_q_1{ping}",
| "operationName": "my_q_1"
| }
""".trimMargin()
)
Expand All @@ -165,7 +206,7 @@ class MicrometerServletSmokeTest {
.and("contextual-tag", "foo")
.and("outcome", "success")
.and("gql.operation", "QUERY")
.and("gql.operation.name", "my_op_1")
.and("gql.operation.name", "my_q_1")
.and("gql.query.complexity", "5")
.and("gql.query.sig.hash", MOCKED_QUERY_SIGNATURE.hash)
)
Expand All @@ -178,7 +219,7 @@ class MicrometerServletSmokeTest {
.and("gql.field", "Query.ping")
.and("outcome", "success")
.and("gql.operation", "QUERY")
.and("gql.operation.name", "my_op_1")
.and("gql.operation.name", "my_q_1")
.and("gql.query.complexity", "5")
.and("gql.query.sig.hash", MOCKED_QUERY_SIGNATURE.hash)
)
Expand Down Expand Up @@ -307,7 +348,7 @@ class MicrometerServletSmokeTest {
.containsAll(
Tags.of("execution-tag", "foo")
.and("contextual-tag", "foo")
.and("gql.operation", "QUERY")
.and("gql.operation", "none")
.and("gql.operation.name", "anonymous")
.and("gql.query.complexity", "none")
.and("gql.query.sig.hash", "none")
Expand All @@ -323,7 +364,7 @@ class MicrometerServletSmokeTest {
Tags.of("execution-tag", "foo")
.and("contextual-tag", "foo")
.and("outcome", "failure")
.and("gql.operation", "QUERY")
.and("gql.operation", "none")
.and("gql.operation.name", "anonymous")
.and("gql.query.complexity", "none")
.and("gql.query.sig.hash", "none")
Expand Down Expand Up @@ -682,6 +723,10 @@ class MicrometerServletSmokeTest {
| triggerCustomFailure: String
|}
|
|type Mutation{
| buzz:String
|}
|
|type StringTransformation {
| index: Int
| value: String
Expand All @@ -697,6 +742,11 @@ class MicrometerServletSmokeTest {
return "pong"
}

@DgsData(parentType = "Mutation", field = "buzz")
fun buzz(): String {
return "buzz"
}

@DgsData(parentType = "Query", field = "transform")
fun transform(@InputArgument input: List<String>): List<Map<String, String>> {
return input.mapIndexed { i, v -> mapOf("index" to "$i", "value" to v) }
Expand Down Expand Up @@ -766,7 +816,7 @@ class MicrometerServletSmokeTest {
val executor = ThreadPoolTaskExecutor()
executor.corePoolSize = 1
executor.maxPoolSize = 1
executor.threadNamePrefix = "${MicrometerServletSmokeTest::class.java.simpleName}-test-"
executor.setThreadNamePrefix("${MicrometerServletSmokeTest::class.java.simpleName}-test-")
executor.setQueueCapacity(10)
executor.initialize()
return executor
Expand Down

0 comments on commit b48f832

Please sign in to comment.