GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Proposal for an additional instrumentation hook point by tinnou · Pull Request #4206 · graphql-java/graphql-java

Conversation

@tinnou

@tinnou tinnou commented Jan 12, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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) on FieldFetchingInstrumentationContext to observe the final DataFetcherResult after exception handling. This is part of the beginFieldFetching instrumentation method callback.

Breaking Changes

  • This is an additive, API backwards-compatible extension. Existing instrumentation remains valid. Instrumentation implementations that want these new capabilities can implement the new interface.
  • the onCompleted method 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:

  1. Extend the existing InstrumentationContext API with new callbacks:
    • Pros: single context type.
    • Cons: would modify the general-purpose context contract and could break or confuse clients; less explicit about field-fetching semantics.
    • Rejected to preserve separation of concerns and minimize API surface changes.
  2. Add separate callbacks on Instrumentation (new methods) rather than a context:
    • Pros: offers a purely additive solution
    • Cons: less flexible for per-field context state and harder to reuse existing InstrumentationContext lifecycle semantics. Adding extra hooks could make the execution feel crowded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tinnou
Add FieldFetchingInstrumentationContext.onExceptionHandled for hookin…
01ace23
…g after exception handling
@bbakerman

bbakerman commented Jan 13, 2026

Copy link
Copy Markdown
Member

@tinnou - long time no see. Let me have a look at this and thanks once again for contributing

tinnou reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bbakerman
bbakerman approved these changes Jan 13, 2026
View reviewed changes

@bbakerman bbakerman left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one - lets add this!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bbakerman
bbakerman merged commit c465601 into graphql-java:master Jan 13, 2026
4 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bbakerman bbakerman added this to the 26.x breaking changes milestone Jan 13, 2026
@tinnou

tinnou commented Jan 13, 2026

Copy link
Copy Markdown
Contributor Author

Nice one - lets add this!

That's some serious speed! ⚡ ⚡ Thank you Brad!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tinnou added a commit to tinnou/graphql-java that referenced this pull request Jan 13, 2026
@tinnou
Add FieldFetchingInstrumentationContext.onExceptionHandled call to Ch…
0bc86c8
…ainedInstrumentation

Missed it from PR graphql-java#4206
tinnou added a commit to tinnou/graphql-java that referenced this pull request Jan 14, 2026
@tinnou
Add FieldFetchingInstrumentationContext.onExceptionHandled call to Ch…
326be4e
…ainedInstrumentation

Missed it from PR graphql-java#4206
tinnou added a commit to tinnou/graphql-java that referenced this pull request Jan 14, 2026
@tinnou
Add FieldFetchingInstrumentationContext.onExceptionHandled call to Ch…
835e9db
…ainedInstrumentation

Missed it from PR graphql-java#4206
@tinnou tinnou mentioned this pull request Jan 14, 2026
Call onExceptionHandled in ChainedInstrumentation #4207
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

26.x is a breaking change release (ot...

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@tinnou @bbakerman
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.