DataLoaderInstrumentation Granularity

[zsmoore]

Hello,

We are updating our infra to move away from custom instrumentation on to the newly provided instrumentation infra.

Had a couple of questions related to this and was looking for thoughts.

We are primarily leveraging Open Telemetry metrics to get realtime data on all of our loaders.
One of the granular parts that we do is by dataloader key in the registry.

This poses a problem when working with the new instrumentation as we only have the dataloader instance here.
One idea i had that I have implemented is doing a reverse map lookup from the dataloaderRegistry to get DataLoader instance -> string cache key for attributes but this poses a problem as instrumentation needs to be provided at build time while the dataloader map is only present after construction resulting in a weird supplier pattern I am writing. I was wondering how we think of possibly providing some dataloader key provider mechanism into instrumentation.

The other question was regarding type safety for instrumentation. Right now we can provide instrumentation on a registry or individual loader level. Would it make sense to provide a type safe instrumentation option if attached at the loader level specifically rather than the registry?

Last, regarding how statistics and instrumentation plays together. We have an OTel Metric sliced like this
Batch Count
Loader name
Batch size

so as we are executing our loaders we get the average batch size that is used on a per-loader basis.

When implementing this via instrumentation I am hooking into the batch function and reporting the batch size but right now it feels a bit weird that we have statistics that are in the ecosystem and present on the dataloader but are a bit weird to work with when it comes to observing them in instrumentation.

Apologies for throwing a bunch of topics into one discussion post but just wondering general thoughts on this.

Thank you!

[bbakerman]

I was wondering how we think of possibly providing some dataloader key provider mechanism into instrumentation.

This makes sense to me. I think because a DataLoader does not name attr on it today I didnt put this in

but we could do

    default DataLoaderInstrumentationContext<Object> beginLoad(@nulllable String dlName, DataLoader<?, ?> dataLoader, Object key, Object loadContext) {

And then if it came from a DLRegistry the it has a name otherwise it does not. This is an idea - I dont if this is feasble in code since a DL does not carry a name

Would it make sense to provide a type safe instrumentation option if attached at the loader level specifically rather than the registry?

I am not sure what this means. I dont know what you mean by a type safe instrumentation option ?? Can you please expand mybe with examples??

When implementing this via instrumentation I am hooking into the batch function and reporting the batch size but right now it feels a bit weird that we have statistics that are in the ecosystem and present on the dataloader but are a bit weird to work with when it comes to observing them in instrumentation.

I dont know this sentence means sorry. I dont know what is weird or hard to do

[zsmoore]

for type safety lets say I have

class A implements BatchLoader<Integer, Integer> {

}

I then have instrumentation I attach specifically to DataLoader<Integer, Integer> that I get from the factory.

In theory our instrumentation interfaces can be

DataLoaderInstrumentation<K, V>

   public DataLoaderInstrumentationContext<DispatchResult<Integer>> beginDispatch(DataLoader<Integer, Integer> dataLoader) {
          return DataLoaderInstrumentationHelper.whenCompleted((result, err) -> {
                    if (result == 0) // some example where you observe the response
          });
   }

This would require quite the refactor of typing DLOptions etc but might unlock something. I don't really have a use case here but was just thinking that we could remove the wildcard generics in the non-registry case if we wanted to

[bbakerman]

The type safety is lost inside the DL code base

Its been

private final Object batchLoadFunction;

since forever - we cant easily retain the generics.

Some casting will be required

[bbakerman]

One idea to help with the naming is to introduce a "nullable" name on the DL. It needs to be nullable so that existing code can work

The DLRegistry then would be able to transform any registered DLs and set in a name if they dont have them and validate they are the sam name as registration

This then makes it so much easier to "carry" this dl name into the code world

[bbakerman]

See #193