[calcAutoInterval] incorporate review feedback

elastic · Oct 27, 2018 · 30b7629 · 30b7629
1 parent b4f8a79
commit 30b7629
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 72 deletions.
diff --git a/src/ui/public/time_buckets/calc_auto_interval.test.ts b/src/ui/public/time_buckets/calc_auto_interval.test.ts
@@ -23,7 +23,7 @@ import { calcAutoIntervalLessThan, calcAutoIntervalNear } from './calc_auto_inte
 
 describe('calcAutoIntervalNear', () => {
   test('0 buckets/1h = 0ms buckets', () => {
-    const interval = calcAutoIntervalNear(0, moment.duration(1, 'h'));
+    const interval = calcAutoIntervalNear(0, Number(moment.duration(1, 'h')));
     expect(interval.asMilliseconds()).toBe(0);
   });
 
@@ -33,54 +33,54 @@ describe('calcAutoIntervalNear', () => {
   });
 
   test('100 buckets/1ms = 1ms buckets', () => {
-    const interval = calcAutoIntervalNear(100, moment.duration(1, 'ms'));
+    const interval = calcAutoIntervalNear(100, Number(moment.duration(1, 'ms')));
     expect(interval.asMilliseconds()).toBe(1);
   });
 
   test('100 buckets/200ms = 2ms buckets', () => {
-    const interval = calcAutoIntervalNear(100, moment.duration(200, 'ms'));
+    const interval = calcAutoIntervalNear(100, Number(moment.duration(200, 'ms')));
     expect(interval.asMilliseconds()).toBe(2);
   });
 
   test('1000 buckets/1s = 1ms buckets', () => {
-    const interval = calcAutoIntervalNear(1000, moment.duration(1, 's'));
+    const interval = calcAutoIntervalNear(1000, Number(moment.duration(1, 's')));
     expect(interval.asMilliseconds()).toBe(1);
   });
 
   test('1000 buckets/1000h = 1h buckets', () => {
-    const interval = calcAutoIntervalNear(1000, moment.duration(1000, 'hours'));
+    const interval = calcAutoIntervalNear(1000, Number(moment.duration(1000, 'hours')));
     expect(interval.asHours()).toBe(1);
   });
 
   test('100 buckets/1h = 30s buckets', () => {
-    const interval = calcAutoIntervalNear(100, moment.duration(1, 'hours'));
+    const interval = calcAutoIntervalNear(100, Number(moment.duration(1, 'hours')));
     expect(interval.asSeconds()).toBe(30);
   });
 
   test('25 buckets/1d = 1h buckets', () => {
-    const interval = calcAutoIntervalNear(25, moment.duration(1, 'day'));
+    const interval = calcAutoIntervalNear(25, Number(moment.duration(1, 'day')));
     expect(interval.asHours()).toBe(1);
   });
 
   test('1000 buckets/1y = 12h buckets', () => {
-    const interval = calcAutoIntervalNear(1000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalNear(1000, Number(moment.duration(1, 'year')));
     expect(interval.asHours()).toBe(12);
   });
 
   test('10000 buckets/1y = 1h buckets', () => {
-    const interval = calcAutoIntervalNear(10000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalNear(10000, Number(moment.duration(1, 'year')));
     expect(interval.asHours()).toBe(1);
   });
 
   test('100000 buckets/1y = 5m buckets', () => {
-    const interval = calcAutoIntervalNear(100000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalNear(100000, Number(moment.duration(1, 'year')));
     expect(interval.asMinutes()).toBe(5);
   });
 });
 
 describe('calcAutoIntervalLessThan', () => {
   test('0 buckets/1h = 0ms buckets', () => {
-    const interval = calcAutoIntervalLessThan(0, moment.duration(1, 'h'));
+    const interval = calcAutoIntervalLessThan(0, Number(moment.duration(1, 'h')));
     expect(interval.asMilliseconds()).toBe(0);
   });
 
@@ -90,47 +90,47 @@ describe('calcAutoIntervalLessThan', () => {
   });
 
   test('100 buckets/1ms = 1ms buckets', () => {
-    const interval = calcAutoIntervalLessThan(100, moment.duration(1, 'ms'));
+    const interval = calcAutoIntervalLessThan(100, Number(moment.duration(1, 'ms')));
     expect(interval.asMilliseconds()).toBe(1);
   });
 
   test('100 buckets/200ms = 2ms buckets', () => {
-    const interval = calcAutoIntervalLessThan(100, moment.duration(200, 'ms'));
+    const interval = calcAutoIntervalLessThan(100, Number(moment.duration(200, 'ms')));
     expect(interval.asMilliseconds()).toBe(2);
   });
 
   test('1000 buckets/1s = 1ms buckets', () => {
-    const interval = calcAutoIntervalLessThan(1000, moment.duration(1, 's'));
+    const interval = calcAutoIntervalLessThan(1000, Number(moment.duration(1, 's')));
     expect(interval.asMilliseconds()).toBe(1);
   });
 
   test('1000 buckets/1000h = 1h buckets', () => {
-    const interval = calcAutoIntervalLessThan(1000, moment.duration(1000, 'hours'));
+    const interval = calcAutoIntervalLessThan(1000, Number(moment.duration(1000, 'hours')));
     expect(interval.asHours()).toBe(1);
   });
 
   test('100 buckets/1h = 30s buckets', () => {
-    const interval = calcAutoIntervalLessThan(100, moment.duration(1, 'hours'));
+    const interval = calcAutoIntervalLessThan(100, Number(moment.duration(1, 'hours')));
     expect(interval.asSeconds()).toBe(30);
   });
 
   test('25 buckets/1d = 30m buckets', () => {
-    const interval = calcAutoIntervalLessThan(25, moment.duration(1, 'day'));
+    const interval = calcAutoIntervalLessThan(25, Number(moment.duration(1, 'day')));
     expect(interval.asMinutes()).toBe(30);
   });
 
   test('1000 buckets/1y = 3h buckets', () => {
-    const interval = calcAutoIntervalLessThan(1000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalLessThan(1000, Number(moment.duration(1, 'year')));
     expect(interval.asHours()).toBe(3);
   });
 
   test('10000 buckets/1y = 30m buckets', () => {
-    const interval = calcAutoIntervalLessThan(10000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalLessThan(10000, Number(moment.duration(1, 'year')));
     expect(interval.asMinutes()).toBe(30);
   });
 
   test('100000 buckets/1y = 5m buckets', () => {
-    const interval = calcAutoIntervalLessThan(100000, moment.duration(1, 'year'));
+    const interval = calcAutoIntervalLessThan(100000, Number(moment.duration(1, 'year')));
     expect(interval.asMinutes()).toBe(5);
   });
 });
diff --git a/src/ui/public/time_buckets/calc_auto_interval.ts b/src/ui/public/time_buckets/calc_auto_interval.ts
@@ -17,82 +17,83 @@
  * under the License.
  */
 
-import moment, { Duration } from 'moment';
+import moment from 'moment';
 
-const rules = [
+const boundsDescending = [
   {
     bound: Infinity,
-    interval: moment.duration(1, 'year'),
+    interval: Number(moment.duration(1, 'year')),
   },
   {
-    bound: moment.duration(1, 'year'),
-    interval: moment.duration(1, 'month'),
+    bound: Number(moment.duration(1, 'year')),
+    interval: Number(moment.duration(1, 'month')),
   },
   {
-    bound: moment.duration(3, 'week'),
-    interval: moment.duration(1, 'week'),
+    bound: Number(moment.duration(3, 'week')),
+    interval: Number(moment.duration(1, 'week')),
   },
   {
-    bound: moment.duration(1, 'week'),
-    interval: moment.duration(1, 'd'),
+    bound: Number(moment.duration(1, 'week')),
+    interval: Number(moment.duration(1, 'd')),
   },
   {
-    bound: moment.duration(24, 'hour'),
-    interval: moment.duration(12, 'hour'),
+    bound: Number(moment.duration(24, 'hour')),
+    interval: Number(moment.duration(12, 'hour')),
   },
   {
-    bound: moment.duration(6, 'hour'),
-    interval: moment.duration(3, 'hour'),
+    bound: Number(moment.duration(6, 'hour')),
+    interval: Number(moment.duration(3, 'hour')),
   },
   {
-    bound: moment.duration(2, 'hour'),
-    interval: moment.duration(1, 'hour'),
+    bound: Number(moment.duration(2, 'hour')),
+    interval: Number(moment.duration(1, 'hour')),
   },
   {
-    bound: moment.duration(45, 'minute'),
-    interval: moment.duration(30, 'minute'),
+    bound: Number(moment.duration(45, 'minute')),
+    interval: Number(moment.duration(30, 'minute')),
   },
   {
-    bound: moment.duration(20, 'minute'),
-    interval: moment.duration(10, 'minute'),
+    bound: Number(moment.duration(20, 'minute')),
+    interval: Number(moment.duration(10, 'minute')),
   },
   {
-    bound: moment.duration(9, 'minute'),
-    interval: moment.duration(5, 'minute'),
+    bound: Number(moment.duration(9, 'minute')),
+    interval: Number(moment.duration(5, 'minute')),
   },
   {
-    bound: moment.duration(3, 'minute'),
-    interval: moment.duration(1, 'minute'),
+    bound: Number(moment.duration(3, 'minute')),
+    interval: Number(moment.duration(1, 'minute')),
   },
   {
-    bound: moment.duration(45, 'second'),
-    interval: moment.duration(30, 'second'),
+    bound: Number(moment.duration(45, 'second')),
+    interval: Number(moment.duration(30, 'second')),
   },
   {
-    bound: moment.duration(15, 'second'),
-    interval: moment.duration(10, 'second'),
+    bound: Number(moment.duration(15, 'second')),
+    interval: Number(moment.duration(10, 'second')),
   },
   {
-    bound: moment.duration(7.5, 'second'),
-    interval: moment.duration(5, 'second'),
+    bound: Number(moment.duration(7.5, 'second')),
+    interval: Number(moment.duration(5, 'second')),
   },
   {
-    bound: moment.duration(5, 'second'),
-    interval: moment.duration(1, 'second'),
+    bound: Number(moment.duration(5, 'second')),
+    interval: Number(moment.duration(1, 'second')),
   },
   {
-    bound: moment.duration(500, 'ms'),
-    interval: moment.duration(100, 'ms'),
+    bound: Number(moment.duration(500, 'ms')),
+    interval: Number(moment.duration(100, 'ms')),
   },
 ];
 
-function estimateBucketMs(count: number, duration: Duration) {
-  const ms = Number(duration) / count;
+function getPerBucketMs(count: number, duration: number) {
+  const ms = duration / count;
   return isFinite(ms) ? ms : NaN;
 }
 
-function defaultInterval(targetMs: number) {
-  return moment.duration(isNaN(targetMs) ? 0 : Math.max(Math.floor(targetMs), 1), 'ms');
+function normalizeMinimumInterval(targetMs: number) {
+  const value = isNaN(targetMs) ? 0 : Math.max(Math.floor(targetMs), 1);
+  return moment.duration(value);
 }
 
 /**
@@ -102,16 +103,24 @@ function defaultInterval(targetMs: number) {
  * @param targetBucketCount desired number of buckets
  * @param duration time range the agg covers
  */
-export function calcAutoIntervalNear(targetBucketCount: number, duration: Duration) {
-  const targetMs = estimateBucketMs(targetBucketCount, duration);
+export function calcAutoIntervalNear(targetBucketCount: number, duration: number) {
+  const targetPerBucketMs = getPerBucketMs(targetBucketCount, duration);
 
-  for (let i = 0; i < rules.length - 1; i++) {
-    if (Number(rules[i + 1].bound) <= targetMs) {
-      return rules[i].interval.clone();
-    }
+  // Find the first bound which is smaller than our target.
+  const lowerBoundIndex = boundsDescending.findIndex(({ bound }) => {
+    const boundMs = Number(bound);
+    return boundMs <= targetPerBucketMs;
+  });
+
+  // The bound immediately preceeding that lower bound contains the
+  // interval most closely matching our target.
+  if (lowerBoundIndex !== -1) {
+    const nearestInterval = boundsDescending[lowerBoundIndex - 1].interval;
+    return moment.duration(nearestInterval);
   }
 
-  return defaultInterval(targetMs);
+  // If the target is smaller than any of our bounds, then we'll use it for the interval as-is.
+  return normalizeMinimumInterval(targetPerBucketMs);
 }
 
 /**
@@ -121,14 +130,16 @@ export function calcAutoIntervalNear(targetBucketCount: number, duration: Durati
  * @param maxBucketCount maximum number of buckets to create
  * @param duration amount of time covered by the agg
  */
-export function calcAutoIntervalLessThan(maxBucketCount: number, duration: Duration) {
-  const maxMs = estimateBucketMs(maxBucketCount, duration);
+export function calcAutoIntervalLessThan(maxBucketCount: number, duration: number) {
+  const maxPerBucketMs = getPerBucketMs(maxBucketCount, duration);
 
-  for (const { interval } of rules) {
-    if (Number(interval) <= maxMs) {
-      return interval.clone();
+  for (const { interval } of boundsDescending) {
+    // Find the highest interval which meets our per bucket limitation.
+    if (interval <= maxPerBucketMs) {
+      return moment.duration(interval);
     }
   }
 
-  return defaultInterval(maxMs);
+  // If the max is smaller than any of our intervals, then we'll use it for the interval as-is.
+  return normalizeMinimumInterval(maxPerBucketMs);
 }
diff --git a/src/ui/public/time_buckets/time_buckets.js b/src/ui/public/time_buckets/time_buckets.js
@@ -231,7 +231,7 @@ TimeBuckets.prototype.getInterval = function (useNormalizedEsInterval = true) {
   function readInterval() {
     const interval = self._i;
     if (moment.isDuration(interval)) return interval;
-    return calcAutoIntervalNear(config.get('histogram:barTarget'), duration);
+    return calcAutoIntervalNear(config.get('histogram:barTarget'), Number(duration));
   }
 
   // check to see if the interval should be scaled, and scale it if so
@@ -243,7 +243,7 @@ TimeBuckets.prototype.getInterval = function (useNormalizedEsInterval = true) {
     let scaled;
 
     if (approxLen > maxLength) {
-      scaled = calcAutoIntervalLessThan(maxLength, duration);
+      scaled = calcAutoIntervalLessThan(maxLength, Number(duration));
     } else {
       return interval;
     }