graphql: Support async data fetchers returning CompletionStage #4896
Conversation
theJC
force-pushed
the
GraphqlCompletableFutureFetcherFix
branch
2 times, most recently
from
abadf3c to
17cae4a
Compare
theJC
force-pushed
the
GraphqlCompletableFutureFetcherFix
branch
6 times, most recently
from
ed3c4bf to
2d044cc
Compare
|
@ygree -- Can I please ask y'all to take a look at this one and see if there is anything else I've missed that is absolutely required to be added before merging this change? I've tried to scope it as small and limited to just the minimal capability we need improved for our needs. I believe after this, there is only one or two last issues before Indeed can transition off its own instrumentation libraries, and just use Datadog's GraphQL tracing instrumentation alone. |
theJC
changed the title
| fieldSpan.finish(); | ||
| throw e; | ||
| } | ||
| if (dataValue instanceof CompletableFuture<?>) { |
There was a problem hiding this comment.
I suggest using CompletionStage instead of CompletableFuture to make this instrumentation work with any CompletionStage implementations other than CompletableFuture.
There was a problem hiding this comment.
Done. Thanks for the recommendation.
...va-14.0/src/main/java/datadog/trace/instrumentation/graphqljava/InstrumentedDataFetcher.java
Show resolved
Hide resolved
Thank you for your contribution! I suggest using CompletionStage instead of CompletableFuture to support other types that implement the CompletionStage interface. Other than that your solution looks good! |
theJC
force-pushed
the
GraphqlCompletableFutureFetcherFix
branch
from
2d044cc to
cf5a7fe
Compare
… adding another stage
theJC
changed the title
theJC
changed the title
|
I've made updates to address all of your feedback and I think we should be all set unless there is feedback on implementation of the updates. Thanks Yury! |
ygree
left a comment
ygree
left a comment
There was a problem hiding this comment.
Looks good! Thanks again for your contribution!
3 participants
What Does This Do
Instruments async data fetchers by adding an additional CompletionStage which finishes the span at the time the future completes
Fixes: #4051
Datadog issue: https://help.datadoghq.com/hc/en-us/requests/986538
Motivation
To allow async fetcher's timeframes be properly represented in APM traces, so that Indeed can retire our custom instrumentation and leverage Datadog's graphql instrumentation instead.
Additional Notes