diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTracker.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTracker.java index c6082300..c1bd3bb0 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTracker.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTracker.java @@ -38,7 +38,9 @@ /** * A tracker used by share usage to attempt to avoid repeatedly sending data - * relating to the same user session. + * relating to the same user session. Evidence that matches an entry is shared + * again once the interval has passed since it was last shared, so a repeat is + * shared at most once in each interval. * @see Specification */ public class ShareUsageTracker extends TrackerBase { @@ -64,7 +66,8 @@ public class ShareUsageTracker extends TrackerBase { * Construct a new instance. * @param configuration the cache configuration to use when building the * internal cache used by this tracker - * @param intervalMillis interval between matched in millis + * @param intervalMillis how long after evidence is shared a repeat of it is + * not shared, in milliseconds * @param filter the {@link EvidenceKeyFilter} that defines the evidence * values to use when creating a key from an {@link FlowData} */ @@ -83,14 +86,22 @@ public ShareUsageTracker( @Override protected Date newValue(FlowData data) { - // Stored meta-data values for this tracker are always true. + // The time the new evidence is shared. return new Date(); } @Override protected boolean match(FlowData data, Date value) { - Date now = new Date(); - return value.getTime() <= (now.getTime() - interval); + // Sharing a repeat stores the time in the cached value, so the next + // interval starts from this share. + long now = System.currentTimeMillis(); + synchronized (value) { + if (value.getTime() <= now - interval) { + value.setTime(now); + return true; + } + return false; + } } @Override diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTrackerTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTrackerTests.java index dff24cba..eaf8670d 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTrackerTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/trackers/ShareUsageTrackerTests.java @@ -40,12 +40,29 @@ import static fiftyone.pipeline.core.Constants.*; import static fiftyone.pipeline.engines.Constants.DEFAULT_SESSION_COOKIE_NAME; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.any; import static org.mockito.Mockito.when; public class ShareUsageTrackerTests { - private long interval; + /** + * An interval no run of these tests comes near, for the tests that do + * not test expiry, so a slow runner cannot make a repeat count as new. + */ + private static final long NO_EXPIRY_INTERVAL_MILLIS = 60 * 60 * 1000; + + /** + * A short interval, for a test that sleeps past it to test expiry. + */ + private static final long EXPIRY_INTERVAL_MILLIS = 50; + + /** + * Added to the wait for an interval to pass, so that a sleep that returns + * early or a slow clock cannot leave a repeat inside the interval. + */ + private static final long EXPIRY_MARGIN_MILLIS = 250; private EvidenceKeyFilter evidenceKeyFilter; @@ -62,7 +79,6 @@ public void Init() { includedQueryStringParameters = new ArrayList<>(); sessionCookieName = DEFAULT_SESSION_COOKIE_NAME; - interval = 50; evidenceKeyFilter = new EvidenceKeyFilterShareUsage( blockedHttpHeaders, includedQueryStringParameters, @@ -73,7 +89,7 @@ public void Init() { 1000); shareUsageTracker = new ShareUsageTracker( cacheConfig, - interval, + NO_EXPIRY_INTERVAL_MILLIS, evidenceKeyFilter); evidenceData = new HashMap<>(); @@ -102,26 +118,56 @@ public void ShareUsageTracker_RepeatEvidence_BeforeSessionTimeout() { @Test public void ShareUsageTracker_RepeatEvidence_AfterSessionTimeout() throws InterruptedException { + ShareUsageTracker shareUsageTracker = new ShareUsageTracker( + new CacheConfiguration(new LruPutCache.Builder(), 1000), + EXPIRY_INTERVAL_MILLIS, + evidenceKeyFilter); int trackedEvents = 0; for (int i = 0; i < 2; i++) { if (shareUsageTracker.track(data)) { trackedEvents++; } - // Wait some time equal to the interval to elapse. - Thread.sleep(interval); + // Wait for the interval to pass, with a margin. + Thread.sleep(EXPIRY_INTERVAL_MILLIS + EXPIRY_MARGIN_MILLIS); } assertEquals(2, trackedEvents); } + /** + * A repeat is shared at most once in each interval. Sharing a repeat after + * the interval starts a new interval, so a repeat straight after it is not + * shared. + */ @Test - public void ShareUsageTracker_Session_Track() throws InterruptedException { - shareUsageTracker = new ShareUsageTracker( + public void ShareUsageTracker_RepeatEvidence_SharedOncePerInterval() + throws InterruptedException { + long interval = 500; + ShareUsageTracker shareUsageTracker = new ShareUsageTracker( new CacheConfiguration(new LruPutCache.Builder(), 1000), interval, evidenceKeyFilter); + assertTrue(shareUsageTracker.track(data), + "New evidence was not shared."); + Thread.sleep(interval + EXPIRY_MARGIN_MILLIS); + assertTrue(shareUsageTracker.track(data), + "A repeat after the interval was not shared."); + assertFalse(shareUsageTracker.track(data), + "A repeat straight after a repeat was shared was also shared, " + + "so the time it was shared was not stored."); + } + + @Test + public void ShareUsageTracker_Session_Track() { + // The interval does not expire, so only the session cookie keeps the + // two requests apart. + shareUsageTracker = new ShareUsageTracker( + new CacheConfiguration(new LruPutCache.Builder(), 1000), + NO_EXPIRY_INTERVAL_MILLIS, + evidenceKeyFilter); + int trackedEvents = 0; for (int i = 0; i < 2; i++) { final Map evidenceData = new HashMap<>(); @@ -138,7 +184,6 @@ public DataKey answer(InvocationOnMock invocationOnMock) throws Throwable { } } ); - Thread.sleep(interval); if (shareUsageTracker.track(data)) { trackedEvents++; } @@ -156,7 +201,7 @@ public void ShareUsageTracker_Session_DoNotTrack() { sessionCookieName); ShareUsageTracker shareUsageTracker = new ShareUsageTracker( new CacheConfiguration(new LruPutCache.Builder(), 1000), - interval, + NO_EXPIRY_INTERVAL_MILLIS, evidenceKeyFilter); int trackedEvents = 0;