Add context objects to StatisticsCollector methods #120
Conversation
| } | ||
|
|
||
| @Override | ||
| public long incrementCacheHitCount() { |
There was a problem hiding this comment.
The diff is a bit clobbered because I decided to order the methods according to the interface.
The `StatisticsCollector` interface is useful for gathering basic metrics about data loaders, but is limited in that we are unable to pass in any sort of context (key or call). This would allow us to obtain more insightful metrics (for example, we would be able to break down metrics based on the GraphQL operation name). To this end, we add `*StatisticsContext` objects to each `StatisticsCollector` method; we provide distinct implementations for each one rather than a generic catch-all so that we may evolve them independently as needed.
| } | ||
|
|
||
| public IncrementBatchLoadCountByStatisticsContext(K key, Object callContext) { | ||
| this.keys = Collections.singletonList(key); |
There was a problem hiding this comment.
use this() to call back to the one place
| this.callContext = callContext; | ||
| } | ||
|
|
||
| public IncrementCacheHitCountStatisticsContext(K key) { |
| * @deprecated use {@link #incrementLoadCount(IncrementLoadCountStatisticsContext)} | ||
| * @return the current value after increment | ||
| */ | ||
| @Deprecated | ||
| long incrementLoadCount(); |
There was a problem hiding this comment.
Nice! Great way to evolve the API
| collector.incrementLoadCount(); | ||
| collector.incrementBatchLoadCountBy(1); | ||
| collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); | ||
| collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); | ||
|
|
||
| BatchLoader<String, String> batchLoader = CompletableFuture::completedFuture; | ||
| DataLoaderOptions loaderOptions = DataLoaderOptions.newOptions().setStatisticsCollector(() -> collector); |
There was a problem hiding this comment.
I think we need a test such that we tets that give a load(K,context) in the data loader, we get K and context passed into the StatsCollector. That is the innards of DataLoader do what we expect them to do
| * @return the current value after increment | ||
| */ | ||
| default <K> long incrementLoadCount(IncrementLoadCountStatisticsContext<K> context) { |
There was a problem hiding this comment.
In hindsight, rather than parameterise each new method, would it have made more sense to parameterise the entire class on K?
There was a problem hiding this comment.
I explored this in a bit more detail; having to pass in a type parameter into certain contexts (ThreadLocalStatisticsCollector's static collector and DataLoaderOptions) made this awkward; so I'll leave this as-is.
The
StatisticsCollectorinterface is useful for gathering basicmetrics about data loaders, but is limited in that we are unable to pass
in any sort of context (key or call). This would allow us to obtain more
insightful metrics (for example, we would be able to break down metrics
based on the GraphQL operation name).
To this end, we add
*StatisticsContextobjects to eachStatisticsCollectormethod; we provide distinct implementations foreach one rather than a generic catch-all so that we may evolve them
independently as needed.