GraphQL java instrumentation #5583
Conversation
| return !draftVersion | ||
| } | ||
|
|
||
| companion object { | ||
| private val GIT_SHA_PATTERN = Regex("^.*-[0-9a-f]{7,}$") | ||
| private val DATETIME_PATTERN = Regex("^\\d{4}-\\d{2}-\\d{2}t\\d{2}-\\d{2}-\\d{2}.*$") |
There was a problem hiding this comment.
graphql-java has a ton of weird versions https://repo1.maven.org/maven2/com/graphql-java/graphql-java/
| @Override | ||
| public void transform(TypeTransformer transformer) { | ||
| transformer.applyAdviceToMethod( | ||
| namedOneOf("checkInstrumentationDefaultState", "checkInstrumentation") |
There was a problem hiding this comment.
These methods add default instrumentations. By applying our instrumentation after these our instrumentation will end up surrounding all other instrumentation.
| public class ExperimentalAttributesExtractor | ||
| implements AttributesExtractor<InstrumentationExecutionParameters, ExecutionResult> { | ||
| // https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-graphql/src/enums/AttributeNames.ts | ||
| private static final AttributeKey<String> OPERATION_NAME = |
There was a problem hiding this comment.
these attributes are the same as in js instrumentation
| import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder; | ||
| import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusExtractor; | ||
|
|
||
| @SuppressWarnings("AbbreviationAsWordInName") |
There was a problem hiding this comment.
as discussed on slack, for library instrumentation I used GraphQL which isn't allowed by our checkstyle rules
|
|
||
| Span span = Span.fromContext(context); | ||
| for (GraphQLError error : result.getErrors()) { | ||
| AttributesBuilder attributes = Attributes.builder(); |
There was a problem hiding this comment.
Record errors (validation and parse errors etc) as exceptions. I don't know whether this is ok.
There was a problem hiding this comment.
I re-read the exceptions spec and it seems ok. Is this what JS implementation does?
There was a problem hiding this comment.
As far as I understand they do something similar with validation errors https://github.com/open-telemetry/opentelemetry-js-contrib/blob/491588f7245e48268bea438cc475e87ae1ed8ad6/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts#L385 but they don't seem to be doing anything with execution errors https://github.com/open-telemetry/opentelemetry-js-contrib/blob/491588f7245e48268bea438cc475e87ae1ed8ad6/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts#L241
| void validationError() { | ||
| ExecutionResult result = | ||
| graphql.execute( | ||
| "" // |
There was a problem hiding this comment.
// prevents spotless from moving these to a single line. Alternatively could use // spotless:off which would need to be enabled with toggleOffOn()
There was a problem hiding this comment.
Out of these two I think I'd probably prefer using // spotless:off for such use cases (though this one works too, I don't have a very strong opinion on that)
There was a problem hiding this comment.
// spotless:off leaves less doubt about what the strange trailing slashes are for 👍
...12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/graphql/GraphqlTracingInstrumentation.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/opentelemetry/instrumentation/graphql/GraphqlTracingInstrumentationState.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/graphql/GraphqlTracingInstrumentation.java
Outdated
Show resolved
Hide resolved
|
|
||
| Span span = Span.fromContext(context); | ||
| for (GraphQLError error : result.getErrors()) { | ||
| AttributesBuilder attributes = Attributes.builder(); |
There was a problem hiding this comment.
I re-read the exceptions spec and it seems ok. Is this what JS implementation does?
.../src/main/java/io/opentelemetry/instrumentation/graphql/ExperimentalAttributesExtractor.java
Outdated
Show resolved
Hide resolved
...java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTracing.java
Outdated
Show resolved
Hide resolved
....0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTracingBuilder.java
Outdated
Show resolved
Hide resolved
...ary/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentationState.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentationState.java
Outdated
Show resolved
Hide resolved
...ary/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java
Outdated
Show resolved
Hide resolved
| void validationError() { | ||
| ExecutionResult result = | ||
| graphql.execute( | ||
| "" // |
There was a problem hiding this comment.
Out of these two I think I'd probably prefer using // spotless:off for such use cases (though this one works too, I don't have a very strong opinion on that)
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
...12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java
Outdated
Show resolved
Hide resolved
| void validationError() { | ||
| ExecutionResult result = | ||
| graphql.execute( | ||
| "" // |
There was a problem hiding this comment.
// spotless:off leaves less doubt about what the strange trailing slashes are for 👍
...ary/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java
Outdated
Show resolved
Hide resolved
...gent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/GraphqlSingletons.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
...ary/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java
Outdated
Show resolved
Hide resolved
|
🥳 |
|
haha cool! |
* GraphQL Java Initial Commit * [WIP] First steps for GraphQL instrumentation, totally not ready [skip ci] * GraphQL Java instrumentation * address review comments * Apply suggestions from code review Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * review feedback * scope handling * Apply suggestions from code review Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> * use spotless:off * trigger build * review comments Co-authored-by: Jordie <xd@jrdie.nl> Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Resolves #4270
Unlike #4337, that attempted to generate the same kind of telemetry as js instrumentation, the instrumentation in this pr generates only a single span for each graphql request. I chose not to follow js implementation as in my opinion it didn't really align with our existing instrumentations. To understand what kind of spans are generated by js implementation it is easiest to look at their test.