[TSVB] metrics:max_buckets setting should limit buckets on server sid…

…e too. (#95639) (#95901) * tmp * [TSVB] Remove metrics:max_buckets setting because it is redundant to histogram:maxBars Closes: #94212 Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
elastic · Mar 31, 2021 · ca51da1 · ca51da1
1 parent 2123d78
commit ca51da1
Show file tree

Hide file tree

Showing 23 changed files with 157 additions and 89 deletions.
diff --git a/...timeseries/common/interval_regexp.test.js → ...timeseries/common/interval_regexp.test.ts b/...timeseries/common/interval_regexp.test.js → ...timeseries/common/interval_regexp.test.ts
diff --git a/...blic/application/lib/validate_interval.js → ...blic/application/lib/validate_interval.ts b/...blic/application/lib/validate_interval.js → ...blic/application/lib/validate_interval.ts
@@ -6,12 +6,20 @@
  * Side Public License, v 1.
  */
 
-import { GTE_INTERVAL_RE } from '../../../common/interval_regexp';
 import { i18n } from '@kbn/i18n';
+import { GTE_INTERVAL_RE } from '../../../common/interval_regexp';
 import { search } from '../../../../../plugins/data/public';
+
+import type { TimeRangeBounds } from '../../../../data/common';
+import type { TimeseriesVisParams } from '../../types';
+
 const { parseInterval } = search.aggs;
 
-export function validateInterval(bounds, panel, maxBuckets) {
+export function validateInterval(
+  bounds: TimeRangeBounds,
+  panel: TimeseriesVisParams,
+  maxBuckets: number
+) {
   const { interval } = panel;
   const { min, max } = bounds;
   // No need to check auto it will return around 100
@@ -20,8 +28,9 @@ export function validateInterval(bounds, panel, maxBuckets) {
   const greaterThanMatch = interval.match(GTE_INTERVAL_RE);
   if (greaterThanMatch) return;
   const duration = parseInterval(interval);
+
   if (duration) {
-    const span = max.valueOf() - min.valueOf();
+    const span = max!.valueOf() - min!.valueOf();
     const buckets = Math.floor(span / duration.asMilliseconds());
     if (buckets > maxBuckets) {
       throw new Error(

diff --git a/src/plugins/vis_type_timeseries/public/request_handler.ts b/src/plugins/vis_type_timeseries/public/request_handler.ts
@@ -28,14 +28,15 @@ export const metricsRequestHandler = async ({
   searchSessionId,
 }: MetricsRequestHandlerParams): Promise<TimeseriesVisData | {}> => {
   const config = getUISettings();
+  const data = getDataStart();
+
   const timezone = getTimezone(config);
   const uiStateObj = uiState[visParams.type] ?? {};
-  const data = getDataStart();
-  const dataSearch = getDataStart().search;
+  const dataSearch = data.search;
   const parsedTimeRange = data.query.timefilter.timefilter.calculateBounds(input?.timeRange!);
 
   if (visParams && visParams.id && !visParams.isModelInvalid) {
-    const maxBuckets = config.get(MAX_BUCKETS_SETTING);
+    const maxBuckets = config.get<number>(MAX_BUCKETS_SETTING);
 
     validateInterval(parsedTimeRange, visParams, maxBuckets);
 

diff --git a/..._timeseries/server/lib/search_strategies/capabilities/default_search_capabilities.test.ts b/..._timeseries/server/lib/search_strategies/capabilities/default_search_capabilities.test.ts
@@ -7,20 +7,19 @@
  */
 
 import { DefaultSearchCapabilities } from './default_search_capabilities';
-import { VisTypeTimeseriesRequest } from '../../../types';
 
 describe('DefaultSearchCapabilities', () => {
   let defaultSearchCapabilities: DefaultSearchCapabilities;
-  let req: VisTypeTimeseriesRequest;
 
   beforeEach(() => {
-    req = {} as VisTypeTimeseriesRequest;
-    defaultSearchCapabilities = new DefaultSearchCapabilities(req);
+    defaultSearchCapabilities = new DefaultSearchCapabilities({
+      timezone: 'UTC',
+      maxBucketsLimit: 2000,
+    });
   });
 
   test('should init default search capabilities', () => {
-    expect(defaultSearchCapabilities.request).toBe(req);
-    expect(defaultSearchCapabilities.fieldsCapabilities).toEqual({});
+    expect(defaultSearchCapabilities.timezone).toBe('UTC');
   });
 
   test('should return defaultTimeInterval', () => {
@@ -35,18 +34,6 @@ describe('DefaultSearchCapabilities', () => {
     });
   });
 
-  test('should return Search Timezone', () => {
-    defaultSearchCapabilities.request = ({
-      body: {
-        timerange: {
-          timezone: 'UTC',
-        },
-      },
-    } as unknown) as VisTypeTimeseriesRequest;
-
-    expect(defaultSearchCapabilities.searchTimezone).toEqual('UTC');
-  });
-
   test('should return a valid time interval', () => {
     expect(defaultSearchCapabilities.getValidTimeInterval('20m')).toBe('20m');
   });

diff --git a/..._type_timeseries/server/lib/search_strategies/capabilities/default_search_capabilities.ts b/..._type_timeseries/server/lib/search_strategies/capabilities/default_search_capabilities.ts
@@ -13,23 +13,20 @@ import {
   getSuitableUnit,
 } from '../../vis_data/helpers/unit_to_seconds';
 import { RESTRICTIONS_KEYS } from '../../../../common/ui_restrictions';
-import { VisTypeTimeseriesRequest, VisTypeTimeseriesVisDataRequest } from '../../../types';
 
-const isVisDataRequest = (
-  request: VisTypeTimeseriesRequest
-): request is VisTypeTimeseriesVisDataRequest => {
-  return !!(request as VisTypeTimeseriesVisDataRequest).body;
-};
-
-const getTimezoneFromRequest = (request: VisTypeTimeseriesRequest) => {
-  if (isVisDataRequest(request)) return request.body.timerange.timezone;
-};
+export interface SearchCapabilitiesOptions {
+  timezone?: string;
+  maxBucketsLimit: number;
+}
 
 export class DefaultSearchCapabilities {
-  constructor(
-    public request: VisTypeTimeseriesRequest,
-    public fieldsCapabilities: Record<string, any> = {}
-  ) {}
+  public timezone: SearchCapabilitiesOptions['timezone'];
+  public maxBucketsLimit: SearchCapabilitiesOptions['maxBucketsLimit'];
+
+  constructor(options: SearchCapabilitiesOptions) {
+    this.timezone = options.timezone;
+    this.maxBucketsLimit = options.maxBucketsLimit;
+  }
 
   public get defaultTimeInterval() {
     return null;
@@ -55,10 +52,6 @@ export class DefaultSearchCapabilities {
     };
   }
 
-  public get searchTimezone() {
-    return getTimezoneFromRequest(this.request);
-  }
-
   createUiRestriction(restrictionsObject?: Record<string, any>) {
     return {
       '*': !restrictionsObject,

diff --git a/...e_timeseries/server/lib/search_strategies/capabilities/rollup_search_capabilities.test.ts b/...e_timeseries/server/lib/search_strategies/capabilities/rollup_search_capabilities.test.ts
@@ -8,13 +8,11 @@
 
 import { Unit } from '@elastic/datemath';
 import { RollupSearchCapabilities } from './rollup_search_capabilities';
-import { VisTypeTimeseriesRequest } from '../../../types';
 
 describe('Rollup Search Capabilities', () => {
   const testTimeZone = 'time_zone';
   const testInterval = '10s';
   const rollupIndex = 'rollupIndex';
-  const request = ({} as unknown) as VisTypeTimeseriesRequest;
 
   let fieldsCapabilities: Record<string, any>;
   let rollupSearchCaps: RollupSearchCapabilities;
@@ -33,16 +31,19 @@ describe('Rollup Search Capabilities', () => {
       },
     };
 
-    rollupSearchCaps = new RollupSearchCapabilities(request, fieldsCapabilities, rollupIndex);
+    rollupSearchCaps = new RollupSearchCapabilities(
+      { maxBucketsLimit: 2000, timezone: 'UTC' },
+      fieldsCapabilities,
+      rollupIndex
+    );
   });
 
   test('should create instance of RollupSearchRequest', () => {
-    expect(rollupSearchCaps.fieldsCapabilities).toBe(fieldsCapabilities);
     expect(rollupSearchCaps.rollupIndex).toBe(rollupIndex);
   });
 
   test('should return the "timezone" for the rollup request', () => {
-    expect(rollupSearchCaps.searchTimezone).toBe(testTimeZone);
+    expect(rollupSearchCaps.timezone).toBe(testTimeZone);
   });
 
   test('should return the default "interval" for the rollup request', () => {

diff --git a/...s_type_timeseries/server/lib/search_strategies/capabilities/rollup_search_capabilities.ts b/...s_type_timeseries/server/lib/search_strategies/capabilities/rollup_search_capabilities.ts
@@ -5,26 +5,28 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-
 import { get, has } from 'lodash';
 import { leastCommonInterval, isCalendarInterval } from '../lib/interval_helper';
 
-import { DefaultSearchCapabilities } from './default_search_capabilities';
-import { VisTypeTimeseriesRequest } from '../../../types';
+import {
+  DefaultSearchCapabilities,
+  SearchCapabilitiesOptions,
+} from './default_search_capabilities';
 
 export class RollupSearchCapabilities extends DefaultSearchCapabilities {
   rollupIndex: string;
   availableMetrics: Record<string, any>;
 
   constructor(
-    req: VisTypeTimeseriesRequest,
+    options: SearchCapabilitiesOptions,
     fieldsCapabilities: Record<string, any>,
     rollupIndex: string
   ) {
-    super(req, fieldsCapabilities);
+    super(options);
 
     this.rollupIndex = rollupIndex;
     this.availableMetrics = get(fieldsCapabilities, `${rollupIndex}.aggs`, {});
+    this.timezone = get(this.dateHistogram, 'time_zone', null);
   }
 
   public get dateHistogram() {
@@ -46,10 +48,6 @@ export class RollupSearchCapabilities extends DefaultSearchCapabilities {
     );
   }
 
-  public get searchTimezone() {
-    return get(this.dateHistogram, 'time_zone', null);
-  }
-
   public get whiteListedMetrics() {
     const baseRestrictions = this.createUiRestriction({
       count: this.createUiRestriction(),

diff --git a/...ugins/vis_type_timeseries/server/lib/search_strategies/search_strategies_registry.test.ts b/...ugins/vis_type_timeseries/server/lib/search_strategies/search_strategies_registry.test.ts
@@ -25,7 +25,15 @@ class MockSearchStrategy extends AbstractSearchStrategy {
 }
 
 describe('SearchStrategyRegister', () => {
-  const requestContext = {} as VisTypeTimeseriesRequestHandlerContext;
+  const requestContext = ({
+    core: {
+      uiSettings: {
+        client: {
+          get: jest.fn(),
+        },
+      },
+    },
+  } as unknown) as VisTypeTimeseriesRequestHandlerContext;
   let registry: SearchStrategyRegistry;
 
   beforeAll(() => {
@@ -44,7 +52,7 @@ describe('SearchStrategyRegister', () => {
   });
 
   test('should return a DefaultSearchStrategy instance', async () => {
-    const req = {} as VisTypeTimeseriesRequest;
+    const req = { body: {} } as VisTypeTimeseriesRequest;
 
     const { searchStrategy, capabilities } = (await registry.getViableStrategy(
       requestContext,
@@ -65,7 +73,7 @@ describe('SearchStrategyRegister', () => {
   });
 
   test('should return a MockSearchStrategy instance', async () => {
-    const req = {} as VisTypeTimeseriesRequest;
+    const req = { body: {} } as VisTypeTimeseriesRequest;
     const anotherSearchStrategy = new MockSearchStrategy();
     registry.addStrategy(anotherSearchStrategy);
 

diff --git a/..._type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.test.ts b/..._type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.test.ts
@@ -33,6 +33,9 @@ describe('AbstractSearchStrategy', () => {
             asCurrentUser: jest.fn(),
           },
         },
+        uiSettings: {
+          client: jest.fn(),
+        },
       },
       search: {
         search: jest.fn().mockReturnValue(from(Promise.resolve({}))),

diff --git a/...s_type_timeseries/server/lib/search_strategies/strategies/default_search_strategy.test.ts b/...s_type_timeseries/server/lib/search_strategies/strategies/default_search_strategy.test.ts
@@ -13,12 +13,23 @@ import {
 import { DefaultSearchStrategy } from './default_search_strategy';
 
 describe('DefaultSearchStrategy', () => {
-  const requestContext = {} as VisTypeTimeseriesRequestHandlerContext;
+  const requestContext = ({
+    core: {
+      uiSettings: {
+        client: {
+          get: jest.fn(),
+        },
+      },
+    },
+  } as unknown) as VisTypeTimeseriesRequestHandlerContext;
+
   let defaultSearchStrategy: DefaultSearchStrategy;
   let req: VisTypeTimeseriesVisDataRequest;
 
   beforeEach(() => {
-    req = {} as VisTypeTimeseriesVisDataRequest;
+    req = {
+      body: {},
+    } as VisTypeTimeseriesVisDataRequest;
     defaultSearchStrategy = new DefaultSearchStrategy();
   });
 
@@ -32,9 +43,11 @@ describe('DefaultSearchStrategy', () => {
     const value = await defaultSearchStrategy.checkForViability(requestContext, req);
 
     expect(value.isViable).toBe(true);
-    expect(value.capabilities).toEqual({
-      request: req,
-      fieldsCapabilities: {},
-    });
+    expect(value.capabilities).toMatchInlineSnapshot(`
+      DefaultSearchCapabilities {
+        "maxBucketsLimit": undefined,
+        "timezone": undefined,
+      }
+    `);
   });
 });
diff --git a/...ns/vis_type_timeseries/server/lib/search_strategies/strategies/default_search_strategy.ts b/...ns/vis_type_timeseries/server/lib/search_strategies/strategies/default_search_strategy.ts
@@ -15,15 +15,21 @@ import type {
   VisTypeTimeseriesRequestHandlerContext,
   VisTypeTimeseriesRequest,
 } from '../../../types';
+import { MAX_BUCKETS_SETTING } from '../../../../common/constants';
 
 export class DefaultSearchStrategy extends AbstractSearchStrategy {
   async checkForViability(
     requestContext: VisTypeTimeseriesRequestHandlerContext,
     req: VisTypeTimeseriesRequest
   ) {
+    const uiSettings = requestContext.core.uiSettings.client;
+
     return {
       isViable: true,
-      capabilities: new DefaultSearchCapabilities(req),
+      capabilities: new DefaultSearchCapabilities({
+        timezone: req.body.timerange?.timezone,
+        maxBucketsLimit: await uiSettings.get(MAX_BUCKETS_SETTING),
+      }),
     };
   }
 

diff --git a/...ins/vis_type_timeseries/server/lib/search_strategies/strategies/rollup_search_strategy.ts b/...ins/vis_type_timeseries/server/lib/search_strategies/strategies/rollup_search_strategy.ts
@@ -17,6 +17,7 @@ import type {
   VisTypeTimeseriesRequestHandlerContext,
   VisTypeTimeseriesVisDataRequest,
 } from '../../../types';
+import { MAX_BUCKETS_SETTING } from '../../../../common/constants';
 
 const getRollupIndices = (rollupData: { [key: string]: any }) => Object.keys(rollupData);
 const isIndexPatternContainsWildcard = (indexPattern: string) => indexPattern.includes('*');
@@ -62,14 +63,21 @@ export class RollupSearchStrategy extends AbstractSearchStrategy {
     ) {
       const rollupData = await this.getRollupData(requestContext, indexPatternString);
       const rollupIndices = getRollupIndices(rollupData);
+      const uiSettings = requestContext.core.uiSettings.client;
 
       isViable = rollupIndices.length === 1;
 
       if (isViable) {
         const [rollupIndex] = rollupIndices;
         const fieldsCapabilities = getCapabilitiesForRollupIndices(rollupData);
 
-        capabilities = new RollupSearchCapabilities(req, fieldsCapabilities, rollupIndex);
+        capabilities = new RollupSearchCapabilities(
+          {
+            maxBucketsLimit: await uiSettings.get(MAX_BUCKETS_SETTING),
+          },
+          fieldsCapabilities,
+          rollupIndex
+        );
       }
     }
 

diff --git a/src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/get_bucket_size.js b/src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/get_bucket_size.js
@@ -30,6 +30,10 @@ const calculateBucketData = (timeInterval, capabilities) => {
     bucketSize = 1;
   }
 
+  if (bucketSize > capabilities.maxBucketsLimit) {
+    bucketSize = capabilities.maxBucketsLimit;
+  }
+
   // Check decimal
   if (parsedInterval && parsedInterval.value % 1 !== 0) {
     if (parsedInterval.unit !== 'ms') {
@@ -61,16 +65,16 @@ const calculateBucketSizeForAutoInterval = (req, maxBars) => {
   return search.aggs.calcAutoIntervalLessThan(maxBars, timerange).asSeconds();
 };
 
-export const getBucketSize = (req, interval, capabilities, maxBars) => {
-  const bucketSize = calculateBucketSizeForAutoInterval(req, maxBars);
-  let intervalString = `${bucketSize}s`;
+export const getBucketSize = (req, interval, capabilities, bars) => {
+  const defaultBucketSize = calculateBucketSizeForAutoInterval(req, bars);
+  let intervalString = `${defaultBucketSize}s`;
 
   const gteAutoMatch = Boolean(interval) && interval.match(GTE_INTERVAL_RE);
 
   if (gteAutoMatch) {
     const bucketData = calculateBucketData(gteAutoMatch[1], capabilities);
 
-    if (bucketData.bucketSize >= bucketSize) {
+    if (bucketData.bucketSize >= defaultBucketSize) {
       return bucketData;
     }
   }