From 5b4eb8cb73c769541e15e984a4dedafd4ead81b0 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 2 Dec 2022 09:04:10 +0100 Subject: [PATCH] Align observation convention with Micrometer best practices This commit ensures that the contextual names of the observations all start with `"graphql"` (lowercase) and that all key values are prefixed with `"graphql.*"` for a cleaner namespace. Fixes gh-558 --- ...faultDataFetcherObservationConvention.java | 2 +- ...ExecutionRequestObservationConvention.java | 5 ++- .../GraphQlObservationDocumentation.java | 26 ++++++++--- ...DataFetcherObservationConventionTests.java | 8 ++-- ...tionRequestObservationConventionTests.java | 12 ++--- ...raphQlObservationInstrumentationTests.java | 44 +++++++++---------- 6 files changed, 55 insertions(+), 42 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConvention.java b/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConvention.java index 2b4ea8fa..c994e8a8 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConvention.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConvention.java @@ -55,7 +55,7 @@ public class DefaultDataFetcherObservationConvention implements DataFetcherObser @Override public String getContextualName(DataFetcherObservationContext context) { - return "graphQL field " + context.getEnvironment().getField().getName(); + return "graphql field " + context.getEnvironment().getField().getName(); } @Override diff --git a/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConvention.java b/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConvention.java index 2b81c17e..49912994 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConvention.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConvention.java @@ -33,7 +33,7 @@ public class DefaultExecutionRequestObservationConvention implements ExecutionRe private static final String DEFAULT_NAME = "graphql.request"; - private static final String BASE_CONTEXTUAL_NAME = "graphQL "; + private static final String BASE_CONTEXTUAL_NAME = "graphql "; private static final KeyValue OUTCOME_SUCCESS = KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OUTCOME, "SUCCESS"); @@ -60,7 +60,8 @@ public class DefaultExecutionRequestObservationConvention implements ExecutionRe @Override public String getContextualName(ExecutionRequestObservationContext context) { - return BASE_CONTEXTUAL_NAME + context.getCarrier().getOperationName(); + String operationName = (context.getCarrier().getOperationName() != null) ? context.getCarrier().getOperationName() : "query"; + return BASE_CONTEXTUAL_NAME + operationName; } @Override diff --git a/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationDocumentation.java b/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationDocumentation.java index c475acf9..ed66dca3 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationDocumentation.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationDocumentation.java @@ -36,6 +36,12 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation * Observation created for GraphQL execution requests. */ EXECUTION_REQUEST { + + @Override + public String getPrefix() { + return "graphql"; + } + @Override public Class> getDefaultConvention() { return DefaultExecutionRequestObservationConvention.class; @@ -58,6 +64,12 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation * data fetching operations. */ DATA_FETCHER { + + @Override + public String getPrefix() { + return "graphql"; + } + @Override public Class> getDefaultConvention() { return DefaultDataFetcherObservationConvention.class; @@ -77,7 +89,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation OUTCOME { @Override public String asString() { - return "outcome"; + return "graphql.outcome"; } }, @@ -87,7 +99,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation OPERATION { @Override public String asString() { - return "operation"; + return "graphql.operation"; } } } @@ -100,7 +112,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation EXECUTION_ID { @Override public String asString() { - return "execution.id"; + return "graphql.execution.id"; } } } @@ -113,7 +125,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation OUTCOME { @Override public String asString() { - return "outcome"; + return "graphql.outcome"; } }, @@ -123,7 +135,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation FIELD_NAME { @Override public String asString() { - return "field.name"; + return "graphql.field.name"; } }, @@ -133,7 +145,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation ERROR_TYPE { @Override public String asString() { - return "error.type"; + return "graphql.error.type"; } } @@ -147,7 +159,7 @@ public enum GraphQlObservationDocumentation implements ObservationDocumentation FIELD_PATH { @Override public String asString() { - return "field.path"; + return "graphql.field.path"; } } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConventionTests.java b/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConventionTests.java index 7dddc575..cdd3f735 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConventionTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultDataFetcherObservationConventionTests.java @@ -59,7 +59,7 @@ class DefaultDataFetcherObservationConventionTests { builder.mergedField(MergedField.newMergedField(Field.newField("project").build()).build()); }); DataFetcherObservationContext context = new DataFetcherObservationContext(environment); - assertThat(this.convention.getContextualName(context)).isEqualTo("graphQL field project"); + assertThat(this.convention.getContextualName(context)).isEqualTo("graphql field project"); } @Test @@ -68,7 +68,7 @@ class DefaultDataFetcherObservationConventionTests { builder.mergedField(MergedField.newMergedField(Field.newField("project").build()).build()); }); DataFetcherObservationContext context = new DataFetcherObservationContext(environment); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("field.name", "project")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.field.name", "project")); } @Test @@ -78,7 +78,7 @@ class DefaultDataFetcherObservationConventionTests { }); DataFetcherObservationContext context = new DataFetcherObservationContext(environment); context.setError(new IllegalStateException("custom data fetching failure")); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("error.type", "IllegalStateException")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.error.type", "IllegalStateException")); } @Test @@ -90,7 +90,7 @@ class DefaultDataFetcherObservationConventionTests { }); DataFetcherObservationContext context = new DataFetcherObservationContext(environment); context.setError(new IllegalStateException("custom data fetching failure")); - assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("field.path", "/projectBySlug/releases")); + assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("graphql.field.path", "/projectBySlug/releases")); } private DataFetchingEnvironment createDataFetchingEnvironment(Consumer consumer) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConventionTests.java b/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConventionTests.java index 2b3f1322..88340b00 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConventionTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConventionTests.java @@ -58,28 +58,28 @@ class DefaultExecutionRequestObservationConventionTests { ExecutionInput input = ExecutionInput.newExecutionInput().query("{ greeting }") .operationName("mutation").build(); ExecutionRequestObservationContext context = createObservationContext(input, builder -> {}); - assertThat(this.convention.getContextualName(context)).isEqualTo("graphQL mutation"); + assertThat(this.convention.getContextualName(context)).isEqualTo("graphql mutation"); } @Test void hasOperationKeyValueWhenSuccessfulOutput() { ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> { }); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("operation", "query")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.operation", "query")); } @Test void hasOutcomeKeyValueWhenSuccessfulOutput() { ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> { }); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("outcome", "SUCCESS")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.outcome", "SUCCESS")); } @Test void hasOutcomeKeyValueWhenErrorOutput() { ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> builder.addError(new QueryOperationMissingError())); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("outcome", "REQUEST_ERROR")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.outcome", "REQUEST_ERROR")); } @Test @@ -87,14 +87,14 @@ class DefaultExecutionRequestObservationConventionTests { ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> { }); context.setError(new IllegalStateException("custom internal error")); - assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("outcome", "INTERNAL_ERROR")); + assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.outcome", "INTERNAL_ERROR")); } @Test void hasExecutionIdKeyValue() { ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> { }); - assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("execution.id", "42")); + assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("graphql.execution.id", "42")); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java b/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java index 21f8403c..7f4edce2 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java @@ -61,16 +61,16 @@ class GraphQlObservationInstrumentationTests { .execute(TestExecutionRequest.forDocument(document)); ResponseHelper response = ResponseHelper.forResponse(responseMono); TestObservationRegistryAssert.assertThat(this.observationRegistry).hasObservationWithNameEqualTo("graphql.request") - .that().hasLowCardinalityKeyValue("outcome", "SUCCESS") - .hasHighCardinalityKeyValueWithKey("execution.id"); + .that().hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS") + .hasHighCardinalityKeyValueWithKey("graphql.execution.id"); TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasNumberOfObservationsWithNameEqualTo("graphql.datafetcher", 1) .hasObservationWithNameEqualTo("graphql.datafetcher") .that() - .hasLowCardinalityKeyValue("outcome", "SUCCESS") - .hasLowCardinalityKeyValue("field.name", "bookById") - .hasHighCardinalityKeyValue("field.path", "/bookById"); + .hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS") + .hasLowCardinalityKeyValue("graphql.field.name", "bookById") + .hasHighCardinalityKeyValue("graphql.field.path", "/bookById"); } @Test @@ -82,8 +82,8 @@ class GraphQlObservationInstrumentationTests { .execute(TestExecutionRequest.forDocument(document)); ResponseHelper response = ResponseHelper.forResponse(responseMono); TestObservationRegistryAssert.assertThat(this.observationRegistry).hasObservationWithNameEqualTo("graphql.request") - .that().hasLowCardinalityKeyValue("outcome", "REQUEST_ERROR") - .hasHighCardinalityKeyValueWithKey("execution.id"); + .that().hasLowCardinalityKeyValue("graphql.outcome", "REQUEST_ERROR") + .hasHighCardinalityKeyValueWithKey("graphql.execution.id"); TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasNumberOfObservationsWithNameEqualTo("graphql.datafetcher", 0); @@ -108,8 +108,8 @@ class GraphQlObservationInstrumentationTests { .execute(TestExecutionRequest.forDocument(document)); ResponseHelper response = ResponseHelper.forResponse(responseMono); TestObservationRegistryAssert.assertThat(this.observationRegistry).hasObservationWithNameEqualTo("graphql.request") - .that().hasLowCardinalityKeyValue("outcome", "SUCCESS") - .hasHighCardinalityKeyValueWithKey("execution.id"); + .that().hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS") + .hasHighCardinalityKeyValueWithKey("graphql.execution.id"); TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasNumberOfObservationsWithNameEqualTo("graphql.datafetcher", 2); @@ -117,13 +117,13 @@ class GraphQlObservationInstrumentationTests { TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasObservationWithNameEqualTo("graphql.datafetcher") .that() - .hasLowCardinalityKeyValue("outcome", "SUCCESS") - .hasLowCardinalityKeyValue("field.name", "bookById") - .hasHighCardinalityKeyValue("field.path", "/bookById"); + .hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS") + .hasLowCardinalityKeyValue("graphql.field.name", "bookById") + .hasHighCardinalityKeyValue("graphql.field.path", "/bookById"); TestObservationRegistryAssert.assertThat(this.observationRegistry) - .hasAnObservationWithAKeyValue("field.name", "author") - .hasAnObservationWithAKeyValue("field.path", "/bookById/author"); + .hasAnObservationWithAKeyValue("graphql.field.name", "author") + .hasAnObservationWithAKeyValue("graphql.field.path", "/bookById/author"); } @Test @@ -148,17 +148,17 @@ class GraphQlObservationInstrumentationTests { .execute(TestExecutionRequest.forDocument(document)); ResponseHelper response = ResponseHelper.forResponse(responseMono); TestObservationRegistryAssert.assertThat(this.observationRegistry).hasObservationWithNameEqualTo("graphql.request") - .that().hasLowCardinalityKeyValue("outcome", "REQUEST_ERROR") - .hasHighCardinalityKeyValueWithKey("execution.id"); + .that().hasLowCardinalityKeyValue("graphql.outcome", "REQUEST_ERROR") + .hasHighCardinalityKeyValueWithKey("graphql.execution.id"); TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasNumberOfObservationsWithNameEqualTo("graphql.datafetcher", 1) .hasObservationWithNameEqualTo("graphql.datafetcher") .that() - .hasLowCardinalityKeyValue("outcome", "ERROR") - .hasLowCardinalityKeyValue("error.type", "IllegalStateException") - .hasLowCardinalityKeyValue("field.name", "bookById") - .hasHighCardinalityKeyValue("field.path", "/bookById"); + .hasLowCardinalityKeyValue("graphql.outcome", "ERROR") + .hasLowCardinalityKeyValue("graphql.error.type", "IllegalStateException") + .hasLowCardinalityKeyValue("graphql.field.name", "bookById") + .hasHighCardinalityKeyValue("graphql.field.path", "/bookById"); } @Test @@ -177,8 +177,8 @@ class GraphQlObservationInstrumentationTests { ResponseHelper response = ResponseHelper.forResponse(responseMono); TestObservationRegistryAssert.assertThat(this.observationRegistry).hasObservationWithNameEqualTo("graphql.request") - .that().hasLowCardinalityKeyValue("outcome", "SUCCESS") - .hasHighCardinalityKeyValueWithKey("execution.id"); + .that().hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS") + .hasHighCardinalityKeyValueWithKey("graphql.execution.id"); TestObservationRegistryAssert.assertThat(this.observationRegistry) .hasObservationWithNameEqualTo("graphql.datafetcher")