Proposal for an additional instrumentation hook point #4206
+189
−8
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CONTEXT
In our infrastructure, we like to capture resolver latency along with error presence on a single metric. This unlocks alerting based on resolver error rate & resolver latency. We also leverage centralized resolver exception handling (via DataFetchingExceptionHandler) and implementations often contains logic to transforms certain classes of exceptions as client or server GraphQL errors. For example a data fetcher might raise a ValidationException which will be caught by the ExceptionHandler and mapped as a client GraphQL error (bad request error type).
Unfortunately right now, the existing instrumentation hooks (e.g instrumentDataFetcher or beginFieldFetching/beginFieldFetch) are insufficient because it's impossible to observe the post-exception-handling
DataFetcherResult. So in the example above, instrumentations would only have access to the ValidationException and not the graphql client error because exception handling has not happened yet. Emitting a metric that tracks the rate of client/server errors as well as latency becomes complicated without replicating the exception handling inside instrumentations.WHAT IS THIS PR PROPOSING?
This PR exposes an additional method on
default void onExceptionHandled(DataFetcherResult<Object> dataFetcherResult)onFieldFetchingInstrumentationContextto observe the finalDataFetcherResultafter exception handling. This is part of thebeginFieldFetchinginstrumentation method callback.Breaking Changes
onCompletedmethod is now called after exception handling, which is a minor change in behavior from the current implementation.I put this PR together as a discussion starting point (using the Cunningham's Law), there are definitive alternatives that I thought of and would love your input.
Alternatives considered:
InstrumentationContextAPI with new callbacks:Instrumentation(new methods) rather than a context:InstrumentationContextlifecycle semantics. Adding extra hooks could make the execution feel crowded.