Can you outline more about your work around?

I am confused mostly by the following and what you mean by instrumentXXX methods. Maybe some pseudo code example might help me.

and all of the instrumentXXX methods take a result, decorate it for each of the instrumentations, and pass that to the super.instrumentXXX methods.

Are you saying you chose one particular instrumentation (DataLoaderDispatcherInstrumentation which is always present) and called N custom instrumentations because you knew they never used complex InstrumentationContext return object?

this only works for instrumentations that do no state management (i.e. return a SimpleInstrumentationContext.noOp() where a whenDispatched or whenCompleted are never called).

yeah so there is 2 aspects to the instrumentation state management. There is the state object created via graphql.execution.instrumentation.Instrumentation#createState(graphql.execution.instrumentation.parameters.InstrumentationCreateStateParameters) and there is the afct you can create a call back InstrumentationContext object that will be called back when the async operation finishes

(a callback from a callback - meta! )

I guess to do what I think you are saying (inline without ever calling back on the individual InstrumentationContext objects we would need some marker interface (sa StatelessInstrumentation marker) and we adjust behavior base on that

but it would be great if we had the ability to differentiate between something that is side-effecting without state (i.e. update a metric)

Also just be clear here - there is probably state involved in a metric update because you have to run a graphql.execution.instrumentation.InstrumentationContext#onCompleted callback to say when the work is "done" say

(for timer metrics etc..)

Can you outline more about your work around?

I am confused mostly by the following and what you mean by instrumentXXX methods. Maybe some pseudo code example might help me.

class FastChainedInstrumentation (
  instrumentations: Array[SimpleInstrumentation])
    extends DataLoaderDispatcherInstrumentation {

  /* Stateless methods that are only side-effecting and do not modify input */

  override def beginExecution(
    parameters: InstrumentationExecutionParameters
  ): InstrumentationContext[ExecutionResult] = {
    instrumentations.foreach(_.beginExecution(parameters))
    super.beginExecution(parameters)
  } 

  ...

  /* Stateful chained methods which decorate an input */

  override def instrumentExecutionContext(
    executionContext: ExecutionContext,
    parameters: InstrumentationExecutionParameters
  ): ExecutionContext = {
    var ec = executionContext
    instrumentations.foreach { inst =>
      ec = inst.instrumentExecutionContext(ec, parameters)
    }
    super.instrumentExecutionContext(ec, parameters)
  }
}

and all of the instrumentXXX methods take a result, decorate it for each of the instrumentations, and pass that to the super.instrumentXXX methods.

Are you saying you chose one particular instrumentation (DataLoaderDispatcherInstrumentation which is always present) and called N custom instrumentations because you knew they never used complex InstrumentationContext return object?

This was specifically to avoid having a ChainedInstrumentation wrap our instrumentations in GraphQL#checkInstrumentationDefaultState, where it will not wrap as long as a DataLoaderDispatcherInstrumentation is present. Even a ChainedInstrumentation with just the default DataLoaderDispatcherInstrumentation and our FastChainedInstrumentation was a significant allocation source given the number of fields being processed.

this only works for instrumentations that do no state management (i.e. return a SimpleInstrumentationContext.noOp() where a whenDispatched or whenCompleted are never called).

yeah so there is 2 aspects to the instrumentation state management. There is the state object created via graphql.execution.instrumentation.Instrumentation#createState(graphql.execution.instrumentation.parameters.InstrumentationCreateStateParameters) and there is the afct you can create a call back InstrumentationContext object that will be called back when the async operation finishes

(a callback from a callback - meta! )

I guess to do what I think you are saying (inline without ever calling back on the individual InstrumentationContext objects we would need some marker interface (sa StatelessInstrumentation marker) and we adjust behavior base on that

Yes!

but it would be great if we had the ability to differentiate between something that is side-effecting without state (i.e. update a metric)

Also just be clear here - there is probably state involved in a metric update because you have to run a graphql.execution.instrumentation.InstrumentationContext#onCompleted callback to say when the work is "done" say

(for timer metrics etc..)

We typically have 5-6 different instrumentations being chained, where they are mostly incrementing a counter based on some state (not a per-invocation timing) or doing some sort of verification (check something via parameters, throw an exception if something if not valid). You are correct that today something that's timing-based would need onDispatched or onCompleted.

as we have scenarios where 45k+ fields are fetched.

you have to give me insight into this - who do you have 45K+ fields in one query . What drives that?

I ask out of morbid curiosity and a desire to see how graphql-java is getting used and hence how it can be improved

Hey @bbakerman,

Thanks for looking into this and the discussion/thoughts around this.

We currently run 15 different pieces of instrumentation. At a high level, here are what they do:

• Dynamic Type Instrumentation (adds some custom objects dynamically at run time for end user defined objects)
  • Uses instrumentSchema method
  • Doesn't rely on onDispatched/onCompleted
• Distributed tracing instrumentation (adds tracing info to response)
  • Uses instrumentExecutionResult
  • Doesn't rely on onDispatched/onCompleted
• Feature flag instrumentation (adds a few fields to the extension around currently active feature flags)
  • Uses instrumentExecutionResult
  • Doesn't rely on onDispatched/onCompleted
• Experiment Instrumentation (adds AB testing info to the extension based on the specific user making the request)
  • Uses instrumentExecutionResult
  • Doesn't rely on onDispatched/onCompleted
• Tracking Instrumentation (this perfroms some async analytics about the requests we are receiving for data analysis)
  • Uses beginParse
  • Doesn't rely on onDispatched/onCompleted - returns SimpleInstrumentationContext.noOp()
• Schema HidingInstrumentation (this is the one we see issues with as it checks every field)
  • Uses instrumentDataFetcher
  • Doesn't rely on onDispatched/onCompleted
• DescriptionIntrospectionFixInstrumentation - temp fix for SDL scalar descriptions are not available for introspection #2410, this can be removed now
  • Uses instrumentDataFetcher
  • Doesn't rely on onDispatched/onCompleted
• Maximum Weight Instrumentation - adds weights to specific queries and prevents queries that are too expensive being used together
  • Uses beginValidation and instrumentExecutionResult
  • Relies on onCompleted
• MaxQueryDepthInstrumentation - from graphql java
• Rate Limiting Instrumentation
  • Uses instrumentExecutionResult and instrumentDataFetcher (to determine if a given query/mutation has rate limiting for it)
  • Doesn't rely on onDispatched/onCompleted
• Require Authentication Instrumentation - validations if a given field/query/mutation requires the user to be authenticated
  • Uses instrumentDataFetcher
  • Doesn't rely on onDispatched/onCompleted
• Volatile Instrumentation - marks a given field as volatile and could be changed later for experimental new changes
  • Uses beginField and instrumentExecutionResult
  • Doesn't rely on onDispatched/onCompleted - adds a field to the instrumentation state in the InstrumentationFieldParameters and returns super.beginField(parameters); in the beginField method
• ResponseTimingInstrumentation - times how long requests take
  • Uses beginExecution
  • Uses onCompleted
• Access Logging Instrumentation - does the equivalent of standard access logs but for graphql, outputting the query and request information, removing sensitives information
  • Uses createState, instrumentDocumentAndVariables, beginExecutionStrategy and instrumentExecutionResult
  • Doesn't rely on onDispatched/onCompleted
• Metrics Recording Instrumentation - logs metrics and timings for different stages of graphql operations
  • Uses beginExecution, beginParse, beginValidation, beginFieldFetch (with customer contexts used to finish the metrics)
  • Doesn't rely on onDispatched/onCompleted

Thanks for the reply. Can you expand on this list (edit the reply maybe) and indicate whether you ever rely on

graphql.execution.instrumentation.InstrumentationContext#onDispatched or graphql.execution.instrumentation.InstrumentationContext#onCompleted running

The reason is this. One idea is to create a "fast instrumentation" method call. Chaining can be simplified if you never need to returned context to call you back.

It can be just a series of call one after the other (discard the need to call back to the context say if an Instrumentation is marked as NoContexNeeded say (future marker interface)

The other way to do that of course is to create a NoContextChainedInstrumentation class say that contains these type of cases and hence it fit inside a Chained INstrumentation say and we require the consumers to wire it together themselves

Updated, we only have the ResponseTime and MaxWeigh Instrumentation that relies on the InstrumentationContext for those 2 methods so your suggestion sounds like but would work in our case for most of our instrumentation.

Is this something that would be possible to get merged in? Or are you thinking of using something more akin to the NoContextChainedInstrumentation or FastChainedInstrumentation mentioned above?

This maybe be merged in at some future time - we are trying to get v18 right now so it wont be soon

As a work around (and perhaps as a permenant strategy) you would be better with specialised FastChainedInstrumentation instead since you are kinda special (super large scale - TB of memory)

I think any future PR would be to "pass state" direct in but it would always run Context callbacks (and hence have to allocate lists to hold them even if they are empty