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.

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

Adds Apollo tracing support into graphql-java #577

Merged
merged 9 commits into from Aug 2, 2017

Conversation

Projects
None yet
3 participants
@bbakerman
Copy link
Member

bbakerman commented Jul 31, 2017

See #576 for a full description

This is dependent on #575 being accepted since it requires the extra instrumentation info to work

bbakerman added some commits Jul 2, 2017

Better context in instrumentation calls.
Now provides ExecutionPath and TypeInfo
Merge remote-tracking branch 'upstream/master' into 573-better-data-f…
…etching-env

# Conflicts:
#	src/main/java/graphql/execution/ExecutionPath.java
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/main/java/graphql/execution/TypeInfo.java
#	src/main/java/graphql/schema/DataFetchingEnvironment.java
#	src/main/java/graphql/schema/DataFetchingEnvironmentBuilder.java
#	src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java

@bbakerman bbakerman requested review from andimarek and kaqqao Jul 31, 2017

@martijnwalraven

This comment has been minimized.

Copy link

martijnwalraven commented Jul 31, 2017

Great to see this moving so fast! One point of feedback is that you may want to use Instant.now() instead of ZonedDateTime.now(), and DateTimeFormatter.ISO_INSTANT instead of DateTimeFormatter.ISO_OFFSET_DATE_TIME for startTime and endTime.

import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters
import graphql.execution.instrumentation.parameters.*

This comment has been minimized.

@bbakerman

bbakerman Aug 1, 2017

Author Member

Bah - some how I have changed my IDEA settings :(

@bbakerman

This comment has been minimized.

Copy link
Member Author

bbakerman commented Aug 1, 2017

@martijnwalraven - you might want to pass that tip onto the Sangria team since they also use DateTimeFormatter.ISO_OFFSET_DATE_TIME. I looked at their code for inspiration

@bbakerman

This comment has been minimized.

Copy link
Member Author

bbakerman commented Aug 1, 2017

Ok moved to DateTimeFormatter.ISO_INSTANT

@martijnwalraven

This comment has been minimized.

Copy link

martijnwalraven commented Aug 1, 2017

@bbakerman: Great! The Scala code has also been changed.

@kaqqao

kaqqao approved these changes Aug 1, 2017

Copy link
Contributor

kaqqao left a comment

Looking great, both code-wise and feature-wise! Makes the instrumentation API much more powerful and adds an awesome feature on top.

@bbakerman bbakerman merged commit ca12e59 into graphql-java:master Aug 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

huangd0ng added a commit to dayuwuxian/graphql-java that referenced this pull request Aug 29, 2017

Adds Apollo tracing support into graphql-java (graphql-java#577)
* Fixed javadoc warning

* Better context in instrumentation calls.

Now provides ExecutionPath and TypeInfo

* Now with just enough change

* Adding Apollo trace support into graphql-java

* Made instrumentation have a createState object to make it (ironically) less stateful as a component

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment