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.

Sign up for GitHub

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

Jump to bottom

Address incorrect operation tag value #519

Merged
merged 1 commit into from
Aug 7, 2021
Merged

Address incorrect operation tag value #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 21 additions & 30 deletions ...ain/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt
View file
Open in desktop
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
Open in desktop
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