#269 - this adds instrumentation to the execution of the graphql query #270
Conversation
src/main/java/graphql/GraphQL.java
Outdated
| this.graphQLSchema = graphQLSchema; | ||
| this.executionStrategy = executionStrategy; | ||
| this.instrumentation = instrumentation; | ||
| } |
There was a problem hiding this comment.
Yet more constructors is an anti pattern I know. I think GraphQL needs a builder
GraphQL graphQL = GraphQL.builder(schema).
.strategy(x)
.instrumentation(y)
.build()
That way the API can be evolved more easily without breaking existing client or adding yet more overloaded methods / constructors
|
|
||
| public Execution(ExecutionStrategy strategy) { | ||
| this.strategy = strategy; | ||
| public Execution(ExecutionStrategy strategy, Instrumentation instrumentation) { |
There was a problem hiding this comment.
I think may of the classes in the package should be package private and hence not API.
This will allow better evolution of the implementation without breaking people. For example I dont think any consumers should be able to create this class. Its really an implementation class
| @@ -13,6 +15,7 @@ | |||
|
|
|||
| public class ExecutionContext { | |||
|
|
|||
| private final Instrumentation instrumentation; | |||
| private GraphQLSchema graphQLSchema; | |||
There was a problem hiding this comment.
The other variables in this class should be final. The setters are an anti pattern.
There was a problem hiding this comment.
#263 outlines this. Immutability for the win
| @@ -0,0 +1,77 @@ | |||
| package graphql.execution.instrumentation; | |||
There was a problem hiding this comment.
InstrumentationContext allows for callback that are site specific and not casting of Object is needed. eg the parse call back gets the parsed document
| * @param arguments the arguments in play | ||
| * @return a non null {@link InstrumentationContext} object that will be called back when the step ends | ||
| */ | ||
| InstrumentationContext<Document> beginParse(String requestString, String operationName, Object context, Map<String, Object> arguments); |
There was a problem hiding this comment.
Having Document passed back and String requestString, String operationName, Object context, Map<String, Object> arguments is a contentious design
On the one hand this allows you to SPY on every thing as it happens and that could be great for better instrumentation.
On the other hand it means the parameters are no API and cant easily be changed.
I think we either do NONE or have a Builder of InstrumentationParseParameters so we can add extra parameters in the future. In fact as I type... this is what I think it should be.
Or.... we have a Map<String,Object> arangement which is not at all type safe but infinitely extendable. Its quasi secret information and might change at any time.
Thoughts?
| * @param document the GraphQL query string | ||
| * @return a non null {@link InstrumentationContext} object that will be called back when the step ends | ||
| */ | ||
| InstrumentationContext<List<ValidationError>> beginValidation(Document document); |
There was a problem hiding this comment.
Same thougths here - Should document be API?
… and hence makes it less brittle for future changes
… and hence makes it less brittle for future changes. Hmm missed test commit
| @@ -4,14 +4,34 @@ | |||
| import graphql.ExecutionResult; | |||
There was a problem hiding this comment.
There are no code hygeine rules for the project so I let IDEA do its full import behaviour of .* imports. This is what we use here in Atlassian and I am a fan of it.
| import graphql.language.Document; | ||
|
|
||
| import java.util.Map; | ||
|
|
There was a problem hiding this comment.
Having Document as API is the most contentious parameter in my book
…tation-to-execution # Conflicts: # src/main/java/graphql/GraphQL.java
… building the top level GraphQL level
…tation-to-execution
…tation-to-execution # Conflicts: # src/main/java/graphql/GraphQL.java
…tation-to-execution
as per #269 the desire here is to be able to measure how long a graphql query is taken
This approach of "return call back " allows for a fairly stateless system if we have multi threaded execution.
So you can "create a timer and then have the end time - start time be calculated via this created object.