From 0558d3ddd1d77692d98e06820f6ac1dac7598a1d Mon Sep 17 00:00:00 2001 From: Alexandre Carlton Date: Sun, 10 Jul 2022 21:35:52 +1000 Subject: [PATCH 1/4] Add context objects to StatisticsCollector methods 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. --- .../java/org/dataloader/DataLoaderHelper.java | 23 +++-- .../stats/DelegatingStatisticsCollector.java | 62 +++++++++--- .../stats/NoOpStatisticsCollector.java | 38 +++++++- .../stats/SimpleStatisticsCollector.java | 49 ++++++++-- .../dataloader/stats/StatisticsCollector.java | 76 +++++++++++++++ .../stats/ThreadLocalStatisticsCollector.java | 62 +++++++++--- ...mentBatchLoadCountByStatisticsContext.java | 28 ++++++ ...chLoadExceptionCountStatisticsContext.java | 22 +++++ ...crementCacheHitCountStatisticsContext.java | 25 +++++ .../IncrementLoadCountStatisticsContext.java | 20 ++++ ...rementLoadErrorCountStatisticsContext.java | 20 ++++ .../org/dataloader/DataLoaderStatsTest.java | 6 +- .../stats/StatisticsCollectorTest.java | 96 ++++++++++--------- 13 files changed, 438 insertions(+), 89 deletions(-) create mode 100644 src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java create mode 100644 src/main/java/org/dataloader/stats/context/IncrementBatchLoadExceptionCountStatisticsContext.java create mode 100644 src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java create mode 100644 src/main/java/org/dataloader/stats/context/IncrementLoadCountStatisticsContext.java create mode 100644 src/main/java/org/dataloader/stats/context/IncrementLoadErrorCountStatisticsContext.java diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index ae22e04..883189c 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -4,6 +4,11 @@ import org.dataloader.annotations.Internal; import org.dataloader.impl.CompletableFutureKit; import org.dataloader.stats.StatisticsCollector; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; import java.time.Clock; import java.time.Instant; @@ -105,7 +110,7 @@ Optional> getIfPresent(K key) { if (cachingEnabled) { Object cacheKey = getCacheKey(nonNull(key)); if (futureCache.containsKey(cacheKey)) { - stats.incrementCacheHitCount(); + stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key)); return Optional.of(futureCache.get(cacheKey)); } } @@ -132,7 +137,7 @@ CompletableFuture load(K key, Object loadContext) { boolean batchingEnabled = loaderOptions.batchingEnabled(); boolean cachingEnabled = loaderOptions.cachingEnabled(); - stats.incrementLoadCount(); + stats.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(key, loadContext)); if (cachingEnabled) { return loadFromCache(key, loadContext, batchingEnabled); @@ -223,7 +228,7 @@ private CompletableFuture> sliceIntoBatchesOfBatches(List keys, List< @SuppressWarnings("unchecked") private CompletableFuture> dispatchQueueBatch(List keys, List callContexts, List> queuedFutures) { - stats.incrementBatchLoadCountBy(keys.size()); + stats.incrementBatchLoadCountBy(keys.size(), new IncrementBatchLoadCountByStatisticsContext<>(keys, callContexts)); CompletableFuture> batchLoad = invokeLoader(keys, callContexts, loaderOptions.cachingEnabled()); return batchLoad .thenApply(values -> { @@ -231,10 +236,12 @@ private CompletableFuture> dispatchQueueBatch(List keys, List List clearCacheKeys = new ArrayList<>(); for (int idx = 0; idx < queuedFutures.size(); idx++) { + K key = keys.get(idx); V value = values.get(idx); + Object callContext = callContexts.get(idx); CompletableFuture future = queuedFutures.get(idx); if (value instanceof Throwable) { - stats.incrementLoadErrorCount(); + stats.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(key, callContext)); future.completeExceptionally((Throwable) value); clearCacheKeys.add(keys.get(idx)); } else if (value instanceof Try) { @@ -244,7 +251,7 @@ private CompletableFuture> dispatchQueueBatch(List keys, List if (tryValue.isSuccess()) { future.complete(tryValue.get()); } else { - stats.incrementLoadErrorCount(); + stats.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(key, callContext)); future.completeExceptionally(tryValue.getThrowable()); clearCacheKeys.add(keys.get(idx)); } @@ -255,7 +262,7 @@ private CompletableFuture> dispatchQueueBatch(List keys, List possiblyClearCacheEntriesOnExceptions(clearCacheKeys); return values; }).exceptionally(ex -> { - stats.incrementBatchLoadExceptionCount(); + stats.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(keys, callContexts)); if (ex instanceof CompletionException) { ex = ex.getCause(); } @@ -294,7 +301,7 @@ private CompletableFuture loadFromCache(K key, Object loadContext, boolean ba if (futureCache.containsKey(cacheKey)) { // We already have a promise for this key, no need to check value cache or queue up load - stats.incrementCacheHitCount(); + stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key, loadContext)); return futureCache.get(cacheKey); } @@ -310,7 +317,7 @@ private CompletableFuture queueOrInvokeLoader(K key, Object loadContext, bool loaderQueue.add(new LoaderQueueEntry<>(key, loadCallFuture, loadContext)); return loadCallFuture; } else { - stats.incrementBatchLoadCountBy(1); + stats.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(key, loadContext)); // immediate execution of batch function return invokeLoaderImmediately(key, loadContext, cachingEnabled); } diff --git a/src/main/java/org/dataloader/stats/DelegatingStatisticsCollector.java b/src/main/java/org/dataloader/stats/DelegatingStatisticsCollector.java index f44c521..f964b29 100644 --- a/src/main/java/org/dataloader/stats/DelegatingStatisticsCollector.java +++ b/src/main/java/org/dataloader/stats/DelegatingStatisticsCollector.java @@ -1,5 +1,11 @@ package org.dataloader.stats; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; + import static org.dataloader.impl.Assertions.nonNull; /** @@ -20,33 +26,63 @@ public DelegatingStatisticsCollector(StatisticsCollector delegateCollector) { } @Override - public long incrementLoadCount() { - delegateCollector.incrementLoadCount(); - return collector.incrementLoadCount(); + public long incrementLoadCount(IncrementLoadCountStatisticsContext context) { + delegateCollector.incrementLoadCount(context); + return collector.incrementLoadCount(context); } + @Deprecated @Override - public long incrementBatchLoadCountBy(long delta) { - delegateCollector.incrementBatchLoadCountBy(delta); - return collector.incrementBatchLoadCountBy(delta); + public long incrementLoadCount() { + return incrementLoadCount(null); } @Override - public long incrementCacheHitCount() { - delegateCollector.incrementCacheHitCount(); - return collector.incrementCacheHitCount(); + public long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { + delegateCollector.incrementLoadErrorCount(context); + return collector.incrementLoadErrorCount(context); } + @Deprecated @Override public long incrementLoadErrorCount() { - delegateCollector.incrementLoadErrorCount(); - return collector.incrementLoadErrorCount(); + return incrementLoadErrorCount(null); + } + + @Override + public long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { + delegateCollector.incrementBatchLoadCountBy(delta, context); + return collector.incrementBatchLoadCountBy(delta, context); + } + + @Deprecated + @Override + public long incrementBatchLoadCountBy(long delta) { + return incrementBatchLoadCountBy(delta, null); + } + + @Override + public long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { + delegateCollector.incrementBatchLoadExceptionCount(context); + return collector.incrementBatchLoadExceptionCount(context); } + @Deprecated @Override public long incrementBatchLoadExceptionCount() { - delegateCollector.incrementBatchLoadExceptionCount(); - return collector.incrementBatchLoadExceptionCount(); + return incrementBatchLoadExceptionCount(null); + } + + @Override + public long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { + delegateCollector.incrementCacheHitCount(context); + return collector.incrementCacheHitCount(context); + } + + @Deprecated + @Override + public long incrementCacheHitCount() { + return incrementCacheHitCount(null); } /** diff --git a/src/main/java/org/dataloader/stats/NoOpStatisticsCollector.java b/src/main/java/org/dataloader/stats/NoOpStatisticsCollector.java index 3c3624f..e7267b3 100644 --- a/src/main/java/org/dataloader/stats/NoOpStatisticsCollector.java +++ b/src/main/java/org/dataloader/stats/NoOpStatisticsCollector.java @@ -1,5 +1,11 @@ package org.dataloader.stats; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; + /** * A statistics collector that does nothing */ @@ -7,29 +13,59 @@ public class NoOpStatisticsCollector implements StatisticsCollector { private static final Statistics ZERO_STATS = new Statistics(); + @Override + public long incrementLoadCount(IncrementLoadCountStatisticsContext context) { + return 0; + } + + @Deprecated @Override public long incrementLoadCount() { + return incrementLoadCount(null); + } + + @Override + public long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { return 0; } + @Deprecated @Override public long incrementLoadErrorCount() { + return incrementLoadErrorCount(null); + } + + @Override + public long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { return 0; } + @Deprecated @Override public long incrementBatchLoadCountBy(long delta) { + return incrementBatchLoadCountBy(delta, null); + } + + @Override + public long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { return 0; } + @Deprecated @Override public long incrementBatchLoadExceptionCount() { + return incrementBatchLoadExceptionCount(null); + } + + @Override + public long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { return 0; } + @Deprecated @Override public long incrementCacheHitCount() { - return 0; + return incrementCacheHitCount(null); } @Override diff --git a/src/main/java/org/dataloader/stats/SimpleStatisticsCollector.java b/src/main/java/org/dataloader/stats/SimpleStatisticsCollector.java index af48b0c..22b3662 100644 --- a/src/main/java/org/dataloader/stats/SimpleStatisticsCollector.java +++ b/src/main/java/org/dataloader/stats/SimpleStatisticsCollector.java @@ -1,5 +1,11 @@ package org.dataloader.stats; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; + import java.util.concurrent.atomic.AtomicLong; /** @@ -17,30 +23,59 @@ public class SimpleStatisticsCollector implements StatisticsCollector { private final AtomicLong loadErrorCount = new AtomicLong(); @Override - public long incrementLoadCount() { + public long incrementLoadCount(IncrementLoadCountStatisticsContext context) { return loadCount.incrementAndGet(); } + @Deprecated + @Override + public long incrementLoadCount() { + return incrementLoadCount(null); + } @Override - public long incrementBatchLoadCountBy(long delta) { + public long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { + return loadErrorCount.incrementAndGet(); + } + + @Deprecated + @Override + public long incrementLoadErrorCount() { + return incrementLoadErrorCount(null); + } + + @Override + public long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { batchInvokeCount.incrementAndGet(); return batchLoadCount.addAndGet(delta); } + @Deprecated @Override - public long incrementCacheHitCount() { - return cacheHitCount.incrementAndGet(); + public long incrementBatchLoadCountBy(long delta) { + return incrementBatchLoadCountBy(delta, null); } @Override - public long incrementLoadErrorCount() { - return loadErrorCount.incrementAndGet(); + public long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { + return batchLoadExceptionCount.incrementAndGet(); } + @Deprecated @Override public long incrementBatchLoadExceptionCount() { - return batchLoadExceptionCount.incrementAndGet(); + return incrementBatchLoadExceptionCount(null); + } + + @Override + public long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { + return cacheHitCount.incrementAndGet(); + } + + @Deprecated + @Override + public long incrementCacheHitCount() { + return incrementCacheHitCount(null); } @Override diff --git a/src/main/java/org/dataloader/stats/StatisticsCollector.java b/src/main/java/org/dataloader/stats/StatisticsCollector.java index 8fde3e4..b32d17e 100644 --- a/src/main/java/org/dataloader/stats/StatisticsCollector.java +++ b/src/main/java/org/dataloader/stats/StatisticsCollector.java @@ -1,6 +1,11 @@ package org.dataloader.stats; import org.dataloader.annotations.PublicSpi; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; /** * This allows statistics to be collected for {@link org.dataloader.DataLoader} operations @@ -11,38 +16,109 @@ public interface StatisticsCollector { /** * Called to increment the number of loads * + * @param the class of the key in the data loader + * @param context the context containing metadata of the data loader invocation + * * @return the current value after increment */ + default long incrementLoadCount(IncrementLoadCountStatisticsContext context) { + return incrementLoadCount(); + } + + /** + * Called to increment the number of loads + * + * @deprecated use {@link #incrementLoadCount(IncrementLoadCountStatisticsContext)} + * @return the current value after increment + */ + @Deprecated long incrementLoadCount(); /** * Called to increment the number of loads that resulted in an object deemed in error * + * @param the class of the key in the data loader + * @param context the context containing metadata of the data loader invocation + * + * @return the current value after increment + */ + default long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { + return incrementLoadErrorCount(); + } + + /** + * Called to increment the number of loads that resulted in an object deemed in error + * + * @deprecated use {@link #incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext)} * @return the current value after increment */ + @Deprecated long incrementLoadErrorCount(); + /** + * Called to increment the number of batch loads + * + * @param the class of the key in the data loader + * @param delta how much to add to the count + * @param context the context containing metadata of the data loader invocation + * + * @return the current value after increment + */ + default long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { + return incrementBatchLoadCountBy(delta); + } + /** * Called to increment the number of batch loads * * @param delta how much to add to the count * + * @deprecated use {@link #incrementBatchLoadCountBy(long, IncrementBatchLoadCountByStatisticsContext)} * @return the current value after increment */ + @Deprecated long incrementBatchLoadCountBy(long delta); /** * Called to increment the number of batch loads exceptions * + * @param the class of the key in the data loader + * @param context the context containing metadata of the data loader invocation + * + * @return the current value after increment + */ + default long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { + return incrementBatchLoadExceptionCount(); + } + + /** + * Called to increment the number of batch loads exceptions + * + * @deprecated use {@link #incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext)} * @return the current value after increment */ + @Deprecated long incrementBatchLoadExceptionCount(); /** * Called to increment the number of cache hits * + * @param the class of the key in the data loader + * @param context the context containing metadata of the data loader invocation + * + * @return the current value after increment + */ + default long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { + return incrementCacheHitCount(); + } + + /** + * Called to increment the number of cache hits + * + * @deprecated use {@link #incrementCacheHitCount(IncrementCacheHitCountStatisticsContext)} * @return the current value after increment */ + @Deprecated long incrementCacheHitCount(); /** diff --git a/src/main/java/org/dataloader/stats/ThreadLocalStatisticsCollector.java b/src/main/java/org/dataloader/stats/ThreadLocalStatisticsCollector.java index ab7b51e..d091c5a 100644 --- a/src/main/java/org/dataloader/stats/ThreadLocalStatisticsCollector.java +++ b/src/main/java/org/dataloader/stats/ThreadLocalStatisticsCollector.java @@ -1,5 +1,11 @@ package org.dataloader.stats; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; + /** * This can collect statistics per thread as well as in an overall sense. This allows you to snapshot stats for a web request say * as well as all requests. @@ -29,33 +35,63 @@ public ThreadLocalStatisticsCollector resetThread() { } @Override - public long incrementLoadCount() { - overallCollector.incrementLoadCount(); - return collector.get().incrementLoadCount(); + public long incrementLoadCount(IncrementLoadCountStatisticsContext context) { + overallCollector.incrementLoadCount(context); + return collector.get().incrementLoadCount(context); } + @Deprecated @Override - public long incrementBatchLoadCountBy(long delta) { - overallCollector.incrementBatchLoadCountBy(delta); - return collector.get().incrementBatchLoadCountBy(delta); + public long incrementLoadCount() { + return incrementLoadCount(null); } @Override - public long incrementCacheHitCount() { - overallCollector.incrementCacheHitCount(); - return collector.get().incrementCacheHitCount(); + public long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { + overallCollector.incrementLoadErrorCount(context); + return collector.get().incrementLoadErrorCount(context); } + @Deprecated @Override public long incrementLoadErrorCount() { - overallCollector.incrementLoadErrorCount(); - return collector.get().incrementLoadErrorCount(); + return incrementLoadErrorCount(null); + } + + @Override + public long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { + overallCollector.incrementBatchLoadCountBy(delta, context); + return collector.get().incrementBatchLoadCountBy(delta, context); + } + + @Deprecated + @Override + public long incrementBatchLoadCountBy(long delta) { + return incrementBatchLoadCountBy(delta, null); + } + + @Override + public long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { + overallCollector.incrementBatchLoadExceptionCount(context); + return collector.get().incrementBatchLoadExceptionCount(context); } + @Deprecated @Override public long incrementBatchLoadExceptionCount() { - overallCollector.incrementBatchLoadExceptionCount(); - return collector.get().incrementBatchLoadExceptionCount(); + return incrementBatchLoadExceptionCount(null); + } + + @Override + public long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { + overallCollector.incrementCacheHitCount(context); + return collector.get().incrementCacheHitCount(context); + } + + @Deprecated + @Override + public long incrementCacheHitCount() { + return incrementCacheHitCount(null); } /** diff --git a/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java new file mode 100644 index 0000000..93a1527 --- /dev/null +++ b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java @@ -0,0 +1,28 @@ +package org.dataloader.stats.context; + +import java.util.Collections; +import java.util.List; + +public class IncrementBatchLoadCountByStatisticsContext { + + private final List keys; + private final List callContexts; + + public IncrementBatchLoadCountByStatisticsContext(List keys, List callContexts) { + this.keys = keys; + this.callContexts = callContexts; + } + + public IncrementBatchLoadCountByStatisticsContext(K key, Object callContext) { + this.keys = Collections.singletonList(key); + this.callContexts = Collections.singletonList(callContext); + } + + public List getKeys() { + return keys; + } + + public List getCallContexts() { + return callContexts; + } +} diff --git a/src/main/java/org/dataloader/stats/context/IncrementBatchLoadExceptionCountStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadExceptionCountStatisticsContext.java new file mode 100644 index 0000000..7f7b50d --- /dev/null +++ b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadExceptionCountStatisticsContext.java @@ -0,0 +1,22 @@ +package org.dataloader.stats.context; + +import java.util.List; + +public class IncrementBatchLoadExceptionCountStatisticsContext { + + private final List keys; + private final List callContexts; + + public IncrementBatchLoadExceptionCountStatisticsContext(List keys, List callContexts) { + this.keys = keys; + this.callContexts = callContexts; + } + + public List getKeys() { + return keys; + } + + public List getCallContexts() { + return callContexts; + } +} diff --git a/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java new file mode 100644 index 0000000..47a54cc --- /dev/null +++ b/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java @@ -0,0 +1,25 @@ +package org.dataloader.stats.context; + +public class IncrementCacheHitCountStatisticsContext { + + private final K key; + private final Object callContext; + + public IncrementCacheHitCountStatisticsContext(K key, Object callContext) { + this.key = key; + this.callContext = callContext; + } + + public IncrementCacheHitCountStatisticsContext(K key) { + this.key = key; + this.callContext = null; + } + + public K getKey() { + return key; + } + + public Object getCallContext() { + return callContext; + } +} diff --git a/src/main/java/org/dataloader/stats/context/IncrementLoadCountStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementLoadCountStatisticsContext.java new file mode 100644 index 0000000..b8f7b46 --- /dev/null +++ b/src/main/java/org/dataloader/stats/context/IncrementLoadCountStatisticsContext.java @@ -0,0 +1,20 @@ +package org.dataloader.stats.context; + +public class IncrementLoadCountStatisticsContext { + + private final K key; + private final Object callContext; + + public IncrementLoadCountStatisticsContext(K key, Object callContext) { + this.key = key; + this.callContext = callContext; + } + + public K getKey() { + return key; + } + + public Object getCallContext() { + return callContext; + } +} diff --git a/src/main/java/org/dataloader/stats/context/IncrementLoadErrorCountStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementLoadErrorCountStatisticsContext.java new file mode 100644 index 0000000..c53d106 --- /dev/null +++ b/src/main/java/org/dataloader/stats/context/IncrementLoadErrorCountStatisticsContext.java @@ -0,0 +1,20 @@ +package org.dataloader.stats.context; + +public class IncrementLoadErrorCountStatisticsContext { + + private final K key; + private final Object callContext; + + public IncrementLoadErrorCountStatisticsContext(K key, Object callContext) { + this.key = key; + this.callContext = callContext; + } + + public K getKey() { + return key; + } + + public Object getCallContext() { + return callContext; + } +} diff --git a/src/test/java/org/dataloader/DataLoaderStatsTest.java b/src/test/java/org/dataloader/DataLoaderStatsTest.java index c32cbc1..94353e1 100644 --- a/src/test/java/org/dataloader/DataLoaderStatsTest.java +++ b/src/test/java/org/dataloader/DataLoaderStatsTest.java @@ -4,6 +4,8 @@ import org.dataloader.stats.SimpleStatisticsCollector; import org.dataloader.stats.Statistics; import org.dataloader.stats.StatisticsCollector; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; import org.junit.Test; import java.util.ArrayList; @@ -63,8 +65,8 @@ public void stats_are_collected_by_default() { public void stats_are_collected_with_specified_collector() { // lets prime it with some numbers so we know its ours StatisticsCollector collector = new SimpleStatisticsCollector(); - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); BatchLoader batchLoader = CompletableFuture::completedFuture; DataLoaderOptions loaderOptions = DataLoaderOptions.newOptions().setStatisticsCollector(() -> collector); diff --git a/src/test/java/org/dataloader/stats/StatisticsCollectorTest.java b/src/test/java/org/dataloader/stats/StatisticsCollectorTest.java index 2b5f5df..fbfd5e2 100644 --- a/src/test/java/org/dataloader/stats/StatisticsCollectorTest.java +++ b/src/test/java/org/dataloader/stats/StatisticsCollectorTest.java @@ -1,9 +1,15 @@ package org.dataloader.stats; +import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; import org.junit.Test; import java.util.concurrent.CompletableFuture; +import static java.util.Collections.singletonList; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; @@ -21,11 +27,11 @@ public void basic_collection() throws Exception { assertThat(collector.getStatistics().getLoadErrorCount(), equalTo(0L)); - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); - collector.incrementBatchLoadExceptionCount(); - collector.incrementLoadErrorCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(singletonList(1), singletonList(null))); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); assertThat(collector.getStatistics().getLoadCount(), equalTo(1L)); assertThat(collector.getStatistics().getBatchLoadCount(), equalTo(1L)); @@ -40,46 +46,46 @@ public void ratios_work() throws Exception { StatisticsCollector collector = new SimpleStatisticsCollector(); - collector.incrementLoadCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); Statistics stats = collector.getStatistics(); assertThat(stats.getBatchLoadRatio(), equalTo(0d)); assertThat(stats.getCacheHitRatio(), equalTo(0d)); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); stats = collector.getStatistics(); assertThat(stats.getBatchLoadRatio(), equalTo(1d / 4d)); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementCacheHitCount(); - collector.incrementCacheHitCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); stats = collector.getStatistics(); assertThat(stats.getCacheHitRatio(), equalTo(2d / 7d)); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementBatchLoadExceptionCount(); - collector.incrementBatchLoadExceptionCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(singletonList(1), singletonList(null))); + collector.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(singletonList(1), singletonList(null))); stats = collector.getStatistics(); assertThat(stats.getBatchLoadExceptionRatio(), equalTo(2d / 10d)); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementLoadCount(); - collector.incrementLoadErrorCount(); - collector.incrementLoadErrorCount(); - collector.incrementLoadErrorCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); stats = collector.getStatistics(); assertThat(stats.getLoadErrorRatio(), equalTo(3d / 13d)); @@ -95,9 +101,9 @@ public void thread_local_collection() throws Exception { assertThat(collector.getStatistics().getCacheHitCount(), equalTo(0L)); - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); assertThat(collector.getStatistics().getLoadCount(), equalTo(1L)); assertThat(collector.getStatistics().getBatchLoadCount(), equalTo(1L)); @@ -109,9 +115,9 @@ public void thread_local_collection() throws Exception { CompletableFuture.supplyAsync(() -> { - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); // per thread stats here assertThat(collector.getStatistics().getLoadCount(), equalTo(1L)); @@ -128,9 +134,9 @@ public void thread_local_collection() throws Exception { // back on this main thread - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); // per thread stats here assertThat(collector.getStatistics().getLoadCount(), equalTo(2L)); @@ -168,11 +174,11 @@ public void delegating_collector_works() throws Exception { assertThat(collector.getStatistics().getCacheMissCount(), equalTo(0L)); - collector.incrementLoadCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); - collector.incrementBatchLoadExceptionCount(); - collector.incrementLoadErrorCount(); + collector.incrementLoadCount(new IncrementLoadCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(singletonList(1), singletonList(null))); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); assertThat(collector.getStatistics().getLoadCount(), equalTo(1L)); assertThat(collector.getStatistics().getBatchLoadCount(), equalTo(1L)); @@ -199,10 +205,10 @@ public void delegating_collector_works() throws Exception { @Test public void noop_is_just_that() throws Exception { StatisticsCollector collector = new NoOpStatisticsCollector(); - collector.incrementLoadErrorCount(); - collector.incrementBatchLoadExceptionCount(); - collector.incrementBatchLoadCountBy(1); - collector.incrementCacheHitCount(); + collector.incrementLoadErrorCount(new IncrementLoadErrorCountStatisticsContext<>(1, null)); + collector.incrementBatchLoadExceptionCount(new IncrementBatchLoadExceptionCountStatisticsContext<>(singletonList(1), singletonList(null))); + collector.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(1, null)); + collector.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(1, null)); assertThat(collector.getStatistics().getLoadCount(), equalTo(0L)); assertThat(collector.getStatistics().getBatchLoadCount(), equalTo(0L)); @@ -210,4 +216,4 @@ public void noop_is_just_that() throws Exception { assertThat(collector.getStatistics().getCacheMissCount(), equalTo(0L)); } -} \ No newline at end of file +} From 8ca99ccdf34b746ccbc216e21d37b103568110c1 Mon Sep 17 00:00:00 2001 From: Alexandre Carlton Date: Sun, 10 Jul 2022 23:19:00 +1000 Subject: [PATCH 2/4] Use this() to call back to a single constructor --- .../context/IncrementBatchLoadCountByStatisticsContext.java | 3 +-- .../stats/context/IncrementCacheHitCountStatisticsContext.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java index 93a1527..13cd2d9 100644 --- a/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java +++ b/src/main/java/org/dataloader/stats/context/IncrementBatchLoadCountByStatisticsContext.java @@ -14,8 +14,7 @@ public IncrementBatchLoadCountByStatisticsContext(List keys, List cal } public IncrementBatchLoadCountByStatisticsContext(K key, Object callContext) { - this.keys = Collections.singletonList(key); - this.callContexts = Collections.singletonList(callContext); + this(Collections.singletonList(key), Collections.singletonList(callContext)); } public List getKeys() { diff --git a/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java b/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java index 47a54cc..2372111 100644 --- a/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java +++ b/src/main/java/org/dataloader/stats/context/IncrementCacheHitCountStatisticsContext.java @@ -11,8 +11,7 @@ public IncrementCacheHitCountStatisticsContext(K key, Object callContext) { } public IncrementCacheHitCountStatisticsContext(K key) { - this.key = key; - this.callContext = null; + this(key, null); } public K getKey() { From 5983e1098627fb567f29b8d8b2d1c7856e2c4c49 Mon Sep 17 00:00:00 2001 From: Alexandre Carlton Date: Sun, 10 Jul 2022 23:25:11 +1000 Subject: [PATCH 3/4] Add test affirming context is passed to collectors --- .../org/dataloader/DataLoaderStatsTest.java | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/src/test/java/org/dataloader/DataLoaderStatsTest.java b/src/test/java/org/dataloader/DataLoaderStatsTest.java index 94353e1..a76d0f7 100644 --- a/src/test/java/org/dataloader/DataLoaderStatsTest.java +++ b/src/test/java/org/dataloader/DataLoaderStatsTest.java @@ -5,7 +5,10 @@ import org.dataloader.stats.Statistics; import org.dataloader.stats.StatisticsCollector; import org.dataloader.stats.context.IncrementBatchLoadCountByStatisticsContext; +import org.dataloader.stats.context.IncrementBatchLoadExceptionCountStatisticsContext; +import org.dataloader.stats.context.IncrementCacheHitCountStatisticsContext; import org.dataloader.stats.context.IncrementLoadCountStatisticsContext; +import org.dataloader.stats.context.IncrementLoadErrorCountStatisticsContext; import org.junit.Test; import java.util.ArrayList; @@ -13,9 +16,11 @@ import java.util.concurrent.CompletableFuture; import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; import static java.util.concurrent.CompletableFuture.completedFuture; import static org.dataloader.DataLoaderFactory.newDataLoader; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.junit.Assert.assertThat; /** @@ -201,4 +206,116 @@ public void stats_are_collected_on_exceptions() { assertThat(stats.getBatchLoadExceptionCount(), equalTo(2L)); assertThat(stats.getLoadErrorCount(), equalTo(3L)); } + + /** + * A simple {@link StatisticsCollector} that stores the contexts passed to it. + */ + private static class ContextPassingStatisticsCollector implements StatisticsCollector { + + public List> incrementLoadCountStatisticsContexts = new ArrayList<>(); + public List> incrementLoadErrorCountStatisticsContexts = new ArrayList<>(); + public List> incrementBatchLoadCountByStatisticsContexts = new ArrayList<>(); + public List> incrementBatchLoadExceptionCountStatisticsContexts = new ArrayList<>(); + public List> incrementCacheHitCountStatisticsContexts = new ArrayList<>(); + + @Override + public long incrementLoadCount(IncrementLoadCountStatisticsContext context) { + incrementLoadCountStatisticsContexts.add(context); + return 0; + } + + @Deprecated + @Override + public long incrementLoadCount() { + return 0; + } + + @Override + public long incrementLoadErrorCount(IncrementLoadErrorCountStatisticsContext context) { + incrementLoadErrorCountStatisticsContexts.add(context); + return 0; + } + + @Deprecated + @Override + public long incrementLoadErrorCount() { + return 0; + } + + @Override + public long incrementBatchLoadCountBy(long delta, IncrementBatchLoadCountByStatisticsContext context) { + incrementBatchLoadCountByStatisticsContexts.add(context); + return 0; + } + + @Deprecated + @Override + public long incrementBatchLoadCountBy(long delta) { + return 0; + } + + @Override + public long incrementBatchLoadExceptionCount(IncrementBatchLoadExceptionCountStatisticsContext context) { + incrementBatchLoadExceptionCountStatisticsContexts.add(context); + return 0; + } + + @Deprecated + @Override + public long incrementBatchLoadExceptionCount() { + return 0; + } + + @Override + public long incrementCacheHitCount(IncrementCacheHitCountStatisticsContext context) { + incrementCacheHitCountStatisticsContexts.add(context); + return 0; + } + + @Deprecated + @Override + public long incrementCacheHitCount() { + return 0; + } + + @Override + public Statistics getStatistics() { + return null; + } + } + + @Test + public void context_is_passed_through_to_collector() { + ContextPassingStatisticsCollector statisticsCollector = new ContextPassingStatisticsCollector(); + DataLoader> loader = newDataLoader(batchLoaderThatBlows, + DataLoaderOptions.newOptions().setStatisticsCollector(() -> statisticsCollector) + ); + + loader.load("key", "keyContext"); + assertThat(statisticsCollector.incrementLoadCountStatisticsContexts, hasSize(1)); + assertThat(statisticsCollector.incrementLoadCountStatisticsContexts.get(0).getKey(), equalTo("key")); + assertThat(statisticsCollector.incrementLoadCountStatisticsContexts.get(0).getCallContext(), equalTo("keyContext")); + + loader.load("key", "keyContext"); + assertThat(statisticsCollector.incrementCacheHitCountStatisticsContexts, hasSize(1)); + assertThat(statisticsCollector.incrementCacheHitCountStatisticsContexts.get(0).getKey(), equalTo("key")); + assertThat(statisticsCollector.incrementCacheHitCountStatisticsContexts.get(0).getCallContext(), equalTo("keyContext")); + + loader.dispatch(); + assertThat(statisticsCollector.incrementBatchLoadCountByStatisticsContexts, hasSize(1)); + assertThat(statisticsCollector.incrementBatchLoadCountByStatisticsContexts.get(0).getKeys(), equalTo(singletonList("key"))); + assertThat(statisticsCollector.incrementBatchLoadCountByStatisticsContexts.get(0).getCallContexts(), equalTo(singletonList("keyContext"))); + + loader.load("exception", "exceptionKeyContext"); + loader.dispatch(); + assertThat(statisticsCollector.incrementBatchLoadExceptionCountStatisticsContexts, hasSize(1)); + assertThat(statisticsCollector.incrementBatchLoadExceptionCountStatisticsContexts.get(0).getKeys(), equalTo(singletonList("exception"))); + assertThat(statisticsCollector.incrementBatchLoadExceptionCountStatisticsContexts.get(0).getCallContexts(), equalTo(singletonList("exceptionKeyContext"))); + + loader.load("error", "errorKeyContext"); + loader.dispatch(); + assertThat(statisticsCollector.incrementLoadErrorCountStatisticsContexts, hasSize(1)); + assertThat(statisticsCollector.incrementLoadErrorCountStatisticsContexts.get(0).getKey(), equalTo("error")); + assertThat(statisticsCollector.incrementLoadErrorCountStatisticsContexts.get(0).getCallContext(), equalTo("errorKeyContext")); + } } From fc2bfaaedaa4f9cb38a123cd9eda102ef25abe8d Mon Sep 17 00:00:00 2001 From: Alexandre Carlton Date: Fri, 15 Jul 2022 12:48:36 +1000 Subject: [PATCH 4/4] Copy across valueCache in DataLoaderOptions copy constructor This looks like an accidental omission, so we include this along with all the other variables inside this class. --- src/main/java/org/dataloader/DataLoaderOptions.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/dataloader/DataLoaderOptions.java b/src/main/java/org/dataloader/DataLoaderOptions.java index c6d47ca..4c79296 100644 --- a/src/main/java/org/dataloader/DataLoaderOptions.java +++ b/src/main/java/org/dataloader/DataLoaderOptions.java @@ -72,6 +72,7 @@ public DataLoaderOptions(DataLoaderOptions other) { this.cachingExceptionsEnabled = other.cachingExceptionsEnabled; this.cacheKeyFunction = other.cacheKeyFunction; this.cacheMap = other.cacheMap; + this.valueCache = other.valueCache; this.maxBatchSize = other.maxBatchSize; this.statisticsCollector = other.statisticsCollector; this.environmentProvider = other.environmentProvider;