From 08d90d037ffae68b5e05f254de21783f8404dccc Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Mon, 11 Mar 2019 21:10:48 -0600 Subject: [PATCH] Ensure extra columns are not shown in table vis when showPartialRows:true (#27154) --- .../interpreter/public/functions/esaggs.js | 2 +- .../table_vis/public/table_vis_params.html | 19 +++++------ .../tabify/__tests__/_integration.js | 2 +- .../agg_response/tabify/_get_columns.js | 10 ++++-- .../agg_response/tabify/_response_writer.js | 19 +++++++---- .../ui/public/agg_response/tabify/tabify.js | 12 ++++++- src/legacy/ui/public/vis/index.d.ts | 2 +- .../ui/public/vis/request_handlers/courier.js | 6 ++-- .../request_handlers/request_handlers.d.ts | 2 +- src/legacy/ui/public/vis/vis.d.ts | 6 +++- .../__snapshots__/build_pipeline.test.js.snap | 4 +++ .../pipeline_helpers/build_pipeline.test.js | 32 +++++++++++++++++-- .../loader/pipeline_helpers/build_pipeline.ts | 28 ++++++++++------ .../visualize/loader/visualize_data_loader.ts | 2 +- test/functional/apps/visualize/_data_table.js | 21 +++++++++++- .../translations/translations/zh-CN.json | 1 - 16 files changed, 125 insertions(+), 43 deletions(-) diff --git a/src/legacy/core_plugins/interpreter/public/functions/esaggs.js b/src/legacy/core_plugins/interpreter/public/functions/esaggs.js index 47679365b9c1..12f2a54223f8 100644 --- a/src/legacy/core_plugins/interpreter/public/functions/esaggs.js +++ b/src/legacy/core_plugins/interpreter/public/functions/esaggs.js @@ -83,7 +83,7 @@ export const esaggs = () => ({ query: get(context, 'query', null), filters: get(context, 'filters', null), forceFetch: true, - isHierarchical: args.metricsAtAllLevels, + metricsAtAllLevels: args.metricsAtAllLevels, partialRows: args.partialRows, inspectorAdapters: handlers.inspectorAdapters, queryFilter, diff --git a/src/legacy/core_plugins/table_vis/public/table_vis_params.html b/src/legacy/core_plugins/table_vis/public/table_vis_params.html index 8ac63c7a1182..4fbd9fd7d7d6 100644 --- a/src/legacy/core_plugins/table_vis/public/table_vis_params.html +++ b/src/legacy/core_plugins/table_vis/public/table_vis_params.html @@ -21,21 +21,18 @@
-
- -
-
diff --git a/src/legacy/ui/public/agg_response/tabify/__tests__/_integration.js b/src/legacy/ui/public/agg_response/tabify/__tests__/_integration.js index e9a99bc33d20..ad6732add354 100644 --- a/src/legacy/ui/public/agg_response/tabify/__tests__/_integration.js +++ b/src/legacy/ui/public/agg_response/tabify/__tests__/_integration.js @@ -138,7 +138,7 @@ describe('tabifyAggResponse Integration', function () { // the default for a non-hierarchical vis is to display // only complete rows, and only put the metrics at the end. - const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { minimalColumns: true }); + const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { metricsAtAllLevels: false }); expectColumns(tabbed, [ext, src, os, avg]); diff --git a/src/legacy/ui/public/agg_response/tabify/_get_columns.js b/src/legacy/ui/public/agg_response/tabify/_get_columns.js index 68df7e1a0333..ac7cb3bcac33 100644 --- a/src/legacy/ui/public/agg_response/tabify/_get_columns.js +++ b/src/legacy/ui/public/agg_response/tabify/_get_columns.js @@ -27,10 +27,16 @@ const getColumn = (agg, i) => { }; }; -export function tabifyGetColumns(aggs, minimal) { +/** + * Builds tabify columns. + * + * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates + * @param {boolean} minimalColumns - setting to true will only return a column for the last bucket/metric instead of one for each level + */ +export function tabifyGetColumns(aggs, minimalColumns) { // pick the columns - if (minimal) { + if (minimalColumns) { return aggs.map((agg, i) => getColumn(agg, i)); } diff --git a/src/legacy/ui/public/agg_response/tabify/_response_writer.js b/src/legacy/ui/public/agg_response/tabify/_response_writer.js index e71fe842e0bc..b23d67b63ae1 100644 --- a/src/legacy/ui/public/agg_response/tabify/_response_writer.js +++ b/src/legacy/ui/public/agg_response/tabify/_response_writer.js @@ -27,20 +27,25 @@ import { tabifyGetColumns } from './_get_columns'; * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates * @param {boolean} metricsAtAllLevels - setting to true will produce metrics for every bucket * @param {boolean} partialRows - setting to true will not remove rows with missing values + * @param {Object} timeRange - time range object, if provided */ -function TabbedAggResponseWriter(aggs, { metricsAtAllLevels = false, partialRows = false, timeRange } = {}) { +function TabbedAggResponseWriter(aggs, { + metricsAtAllLevels = false, + partialRows = false, + timeRange +} = {}) { + // Private + this._removePartialRows = !partialRows; + + // Public this.rowBuffer = {}; this.bucketBuffer = []; this.metricBuffer = []; - - this.metricsForAllBuckets = metricsAtAllLevels; - this.partialRows = partialRows; this.aggs = aggs; + this.partialRows = partialRows; this.columns = tabifyGetColumns(aggs.getResponseAggs(), !metricsAtAllLevels); this.aggStack = [...this.columns]; - this.rows = []; - // Extract the time range object if provided if (timeRange) { const timeRangeKey = Object.keys(timeRange)[0]; @@ -67,7 +72,7 @@ TabbedAggResponseWriter.prototype.row = function () { this.rowBuffer[metric.id] = metric.value; }); - if (!toArray(this.rowBuffer).length || (!this.partialRows && this.isPartialRow(this.rowBuffer))) { + if (!toArray(this.rowBuffer).length || (this._removePartialRows && this.isPartialRow(this.rowBuffer))) { return; } diff --git a/src/legacy/ui/public/agg_response/tabify/tabify.js b/src/legacy/ui/public/agg_response/tabify/tabify.js index 08abe3fbb359..16374ecd6d52 100644 --- a/src/legacy/ui/public/agg_response/tabify/tabify.js +++ b/src/legacy/ui/public/agg_response/tabify/tabify.js @@ -21,6 +21,16 @@ import _ from 'lodash'; import { TabbedAggResponseWriter } from './_response_writer'; import { TabifyBuckets } from './_buckets'; +/** + * Sets up the ResponseWriter and kicks off bucket collection. + * + * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates + * @param {Object} esResponse - response that came back from Elasticsearch + * @param {Object} respOpts - options object for the ResponseWriter with params set by Courier + * @param {boolean} respOpts.metricsAtAllLevels - setting to true will produce metrics for every bucket + * @param {boolean} respOpts.partialRows - setting to true will not remove rows with missing values + * @param {Object} respOpts.timeRange - time range object, if provided + */ export function tabifyAggResponse(aggs, esResponse, respOpts = {}) { const write = new TabbedAggResponseWriter(aggs, respOpts); @@ -34,7 +44,7 @@ export function tabifyAggResponse(aggs, esResponse, respOpts = {}) { } /** - * read an aggregation from a bucket, which is *might* be found at key (if + * read an aggregation from a bucket, which *might* be found at key (if * the response came in object form), and will recurse down the aggregation * tree and will pass the read values to the ResponseWriter. * diff --git a/src/legacy/ui/public/vis/index.d.ts b/src/legacy/ui/public/vis/index.d.ts index 4b8295367a32..e5d9131bd0fb 100644 --- a/src/legacy/ui/public/vis/index.d.ts +++ b/src/legacy/ui/public/vis/index.d.ts @@ -18,7 +18,7 @@ */ export { AggConfig } from './agg_config'; -export { Vis, VisProvider, VisState } from './vis'; +export { Vis, VisProvider, VisParams, VisState } from './vis'; export { VisualizationController, VisType } from './vis_types/vis_type'; export * from './request_handlers'; export * from './response_handlers'; diff --git a/src/legacy/ui/public/vis/request_handlers/courier.js b/src/legacy/ui/public/vis/request_handlers/courier.js index 01bfc5d84001..80f842534019 100644 --- a/src/legacy/ui/public/vis/request_handlers/courier.js +++ b/src/legacy/ui/public/vis/request_handlers/courier.js @@ -38,7 +38,7 @@ const CourierRequestHandlerProvider = function () { filters, forceFetch, partialRows, - isHierarchical, + metricsAtAllLevels, inspectorAdapters, queryFilter }) { @@ -67,7 +67,7 @@ const CourierRequestHandlerProvider = function () { }); requestSearchSource.setField('aggs', function () { - return aggs.toDsl(isHierarchical); + return aggs.toDsl(metricsAtAllLevels); }); requestSearchSource.onRequestStart((searchSource, searchRequest) => { @@ -143,7 +143,7 @@ const CourierRequestHandlerProvider = function () { const parsedTimeRange = timeRange ? getTime(aggs.indexPattern, timeRange) : null; const tabifyParams = { - metricsAtAllLevels: isHierarchical, + metricsAtAllLevels, partialRows, timeRange: parsedTimeRange ? parsedTimeRange.range : undefined, }; diff --git a/src/legacy/ui/public/vis/request_handlers/request_handlers.d.ts b/src/legacy/ui/public/vis/request_handlers/request_handlers.d.ts index e6f93c5353f9..697072a0de08 100644 --- a/src/legacy/ui/public/vis/request_handlers/request_handlers.d.ts +++ b/src/legacy/ui/public/vis/request_handlers/request_handlers.d.ts @@ -36,7 +36,7 @@ export interface RequestHandlerParams { uiState?: PersistedState; partialRows?: boolean; inspectorAdapters?: Adapters; - isHierarchical?: boolean; + metricsAtAllLevels?: boolean; visParams?: any; } diff --git a/src/legacy/ui/public/vis/vis.d.ts b/src/legacy/ui/public/vis/vis.d.ts index b5fb5390bf86..9e6107ed7594 100644 --- a/src/legacy/ui/public/vis/vis.d.ts +++ b/src/legacy/ui/public/vis/vis.d.ts @@ -31,9 +31,13 @@ export interface Vis { export type VisProvider = (...dependencies: any[]) => Vis; +export interface VisParams { + [key: string]: any; +} + export interface VisState { title: string; type: VisType; - params: any; + params: VisParams; aggs: any[]; } diff --git a/src/legacy/ui/public/visualize/loader/pipeline_helpers/__snapshots__/build_pipeline.test.js.snap b/src/legacy/ui/public/visualize/loader/pipeline_helpers/__snapshots__/build_pipeline.test.js.snap index ae592282316e..0456af8ff5ec 100644 --- a/src/legacy/ui/public/visualize/loader/pipeline_helpers/__snapshots__/build_pipeline.test.js.snap +++ b/src/legacy/ui/public/visualize/loader/pipeline_helpers/__snapshots__/build_pipeline.test.js.snap @@ -16,6 +16,10 @@ exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunct exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunction handles region_map function without buckets 1`] = `"regionmap visConfig='{\\"metric\\":0}' "`; +exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunction handles table function with showPartialRows=true and showMetricsAtAllLevels=false 1`] = `"kibana_table visConfig='{\\"showMetricsAtAllLevels\\":false,\\"showPartialRows\\":true,\\"dimensions\\":{\\"metrics\\":[4,5],\\"buckets\\":[0,3]}}' "`; + +exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunction handles table function with showPartialRows=true and showMetricsAtAllLevels=true 1`] = `"kibana_table visConfig='{\\"showMetricsAtAllLevels\\":true,\\"showPartialRows\\":true,\\"dimensions\\":{\\"metrics\\":[1,2,4,5],\\"buckets\\":[0,3]}}' "`; + exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunction handles table function with splits 1`] = `"kibana_table visConfig='{\\"foo\\":\\"bar\\",\\"dimensions\\":{\\"metrics\\":[0],\\"buckets\\":[],\\"splitRow\\":[1,2]}}' "`; exports[`visualize loader pipeline helpers: build pipeline buildPipelineVisFunction handles table function with splits and buckets 1`] = `"kibana_table visConfig='{\\"foo\\":\\"bar\\",\\"dimensions\\":{\\"metrics\\":[0,1],\\"buckets\\":[3],\\"splitRow\\":[2,4]}}' "`; diff --git a/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.test.js b/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.test.js index 01c10e65f503..50aafbe2848c 100644 --- a/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.test.js +++ b/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.test.js @@ -93,14 +93,15 @@ describe('visualize loader pipeline helpers: build pipeline', () => { }); describe('handles table function', () => { - const params = { foo: 'bar' }; it('without splits or buckets', () => { + const params = { foo: 'bar' }; const schemas = { metric: [0, 1] }; const actual = buildPipelineVisFunction.table({ params }, schemas); expect(actual).toMatchSnapshot(); }); it('with splits', () => { + const params = { foo: 'bar' }; const schemas = { metric: [0], split_row: [1, 2], @@ -110,10 +111,37 @@ describe('visualize loader pipeline helpers: build pipeline', () => { }); it('with splits and buckets', () => { + const params = { foo: 'bar' }; const schemas = { metric: [0, 1], split_row: [2, 4], - bucket: [3] + bucket: [3], + }; + const actual = buildPipelineVisFunction.table({ params }, schemas); + expect(actual).toMatchSnapshot(); + }); + + it('with showPartialRows=true and showMetricsAtAllLevels=true', () => { + const params = { + showMetricsAtAllLevels: true, + showPartialRows: true, + }; + const schemas = { + metric: [1, 2, 4, 5], + bucket: [0, 3], + }; + const actual = buildPipelineVisFunction.table({ params }, schemas); + expect(actual).toMatchSnapshot(); + }); + + it('with showPartialRows=true and showMetricsAtAllLevels=false', () => { + const params = { + showMetricsAtAllLevels: false, + showPartialRows: true, + }; + const schemas = { + metric: [1, 2, 4, 5], + bucket: [0, 3], }; const actual = buildPipelineVisFunction.table({ params }, schemas); expect(actual).toMatchSnapshot(); diff --git a/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts b/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts index a855f2489577..27dc559a4bef 100644 --- a/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts +++ b/src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts @@ -21,7 +21,7 @@ import { cloneDeep } from 'lodash'; // @ts-ignore import { setBounds } from 'ui/agg_types/buckets/date_histogram'; import { SearchSource } from 'ui/courier'; -import { AggConfig, Vis, VisState } from 'ui/vis'; +import { AggConfig, Vis, VisParams, VisState } from 'ui/vis'; interface SchemaFormat { id: string; @@ -51,7 +51,7 @@ interface Schemas { } type buildVisFunction = (visState: VisState, schemas: Schemas, uiState: any) => string; -type buildVisConfigFunction = (schemas: Schemas) => VisState; +type buildVisConfigFunction = (schemas: Schemas, visParams?: VisParams) => VisParams; interface BuildPipelineVisFunction { [key: string]: buildVisFunction; @@ -226,7 +226,7 @@ export const buildPipelineVisFunction: BuildPipelineVisFunction = { table: (visState, schemas) => { const visConfig = { ...visState.params, - ...buildVisConfig.table(schemas), + ...buildVisConfig.table(schemas, visState.params), }; return `kibana_table ${prepareJson('visConfig', visConfig)}`; }, @@ -268,14 +268,24 @@ export const buildPipelineVisFunction: BuildPipelineVisFunction = { }; const buildVisConfig: BuildVisConfigFunction = { - table: schemas => { + table: (schemas, visParams = {}) => { const visConfig = {} as any; + const metrics = schemas.metric; + const buckets = schemas.bucket || []; visConfig.dimensions = { - metrics: schemas.metric, - buckets: schemas.bucket || [], + metrics, + buckets, splitRow: schemas.split_row, splitColumn: schemas.split_column, }; + + if (visParams.showMetricsAtAllLevels === false && visParams.showPartialRows === true) { + // Handle case where user wants to see partial rows but not metrics at all levels. + // This requires calculating how many metrics will come back in the tabified response, + // and removing all metrics from the dimensions except the last set. + const metricsPerBucket = metrics.length / buckets.length; + visConfig.dimensions.metrics.splice(0, metricsPerBucket * buckets.length - metricsPerBucket); + } return visConfig; }, metric: schemas => { @@ -355,7 +365,7 @@ export const getVisParams = (vis: Vis, params: { timeRange?: any }) => { if (buildVisConfig[vis.type.name]) { visConfig = { ...visConfig, - ...buildVisConfig[vis.type.name](schemas), + ...buildVisConfig[vis.type.name](schemas, visConfig), }; } else if (vislibCharts.includes(vis.type.name)) { visConfig.dimensions = buildVislibDimensions(vis, params.timeRange); @@ -392,7 +402,7 @@ export const buildPipeline = ( pipeline += `esaggs ${prepareString('index', indexPattern.id)} metricsAtAllLevels=${vis.isHierarchical()} - partialRows=${vis.params.showPartialRows || vis.type.requiresPartialRows || false} + partialRows=${vis.type.requiresPartialRows || vis.params.showPartialRows || false} ${prepareJson('aggConfigs', visState.aggs)} | `; } @@ -408,7 +418,7 @@ export const buildPipeline = ( pipeline += `visualization type='${vis.type.name}' ${prepareJson('visConfig', visState.params)} metricsAtAllLevels=${vis.isHierarchical()} - partialRows=${vis.params.showPartialRows || vis.type.name === 'tile_map'} `; + partialRows=${vis.type.requiresPartialRows || vis.params.showPartialRows || false} `; if (indexPattern) { pipeline += `${prepareString('index', indexPattern.id)}`; } diff --git a/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts b/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts index b8506bf417d2..c64a0c385a10 100644 --- a/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts +++ b/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts @@ -73,7 +73,7 @@ export class VisualizeDataLoader { // searchSource is only there for courier request handler const requestHandlerResponse = await this.requestHandler({ partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows, - isHierarchical: this.vis.isHierarchical(), + metricsAtAllLevels: this.vis.isHierarchical(), visParams, ...params, filters: params.filters ? params.filters.filter(filter => !filter.meta.disabled) : undefined, diff --git a/test/functional/apps/visualize/_data_table.js b/test/functional/apps/visualize/_data_table.js index f2f8b06e5a41..16942dc26e57 100644 --- a/test/functional/apps/visualize/_data_table.js +++ b/test/functional/apps/visualize/_data_table.js @@ -277,6 +277,25 @@ export default function ({ getService, getPageObjects }) { ]); }); + it('should show correct data without showMetricsAtAllLevels even if showPartialRows is selected', async () => { + await PageObjects.visualize.clickOptionsTab(); + await PageObjects.visualize.checkCheckbox('showPartialRows'); + await PageObjects.visualize.clickGo(); + const data = await PageObjects.visualize.getTableVisContent(); + expect(data).to.be.eql([ + [ 'jpg', 'CN', '1,718' ], + [ 'jpg', 'IN', '1,511' ], + [ 'jpg', 'US', '770' ], + [ 'jpg', 'ID', '314' ], + [ 'jpg', 'PK', '244' ], + [ 'css', 'CN', '422' ], + [ 'css', 'IN', '346' ], + [ 'css', 'US', '189' ], + [ 'css', 'ID', '68' ], + [ 'css', 'BR', '58' ], + ]); + }); + it('should show metrics on each level', async () => { await PageObjects.visualize.clickOptionsTab(); await PageObjects.visualize.checkCheckbox('showMetricsAtAllLevels'); @@ -374,7 +393,7 @@ export default function ({ getService, getPageObjects }) { ]); }); - it('should not show metrics for split bucket when using showMetricsAtAllLevels', async () => { + it('should show metrics for split bucket when using showMetricsAtAllLevels', async () => { await PageObjects.visualize.clickOptionsTab(); await PageObjects.visualize.checkCheckbox('showMetricsAtAllLevels'); await PageObjects.visualize.clickGo(); diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 668ed96b08c2..bec68cb367bd 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -2334,7 +2334,6 @@ "statusPage.statusApp.statusTitle": "插件状态", "statusPage.statusTable.columns.idHeader": "ID", "statusPage.statusTable.columns.statusHeader": "状态", - "tableVis.params.calculateMetricsLabel": "计算每个桶/级别的指标", "tableVis.params.perPageLabel": "每页", "tableVis.params.showMetricsLabel": "显示每个桶/级别的指标", "tableVis.params.showPartialRowsLabel": "显示部分行",