From 5ffbc2658150852cd4f20489c2619725f110b69c Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Fri, 11 Aug 2023 11:52:17 +0200 Subject: [PATCH] [Lens] Relax counter field checks for saved visualizations with unsupported operations (#163515) ## Summary Fix #163473 This PR relaxes a bit the checks on the Lens side for old/saved visualizations with unsupported operations for the `counter` field type, while preserving those checks for newer visualizations. Dashboards with "meaningless" operations will now show a warning message: Screenshot 2023-08-09 at 18 31 21 When in editor the warning is shown at the top-right corner as well: Screenshot 2023-08-09 at 18 30 31 New visualizations still prevent the user from using the unsupported operations: Screenshot 2023-08-09 at 18 30 55 Screenshot 2023-08-09 at 18 31 48 There's theoretically a last case where users in old SOs might create a new metric dimension trying to force to use a unsupported operation for a counter field: in this case the logic for a "new" visualization will kick-in, clean the data in the workspace and show a full error. Cancelling such metric dimension will lead to the previous "relaxed" state. Messages are grouped by field and by top referencing column (i.e. a formula): this means that if a formula uses the same `counter` field with two different dimensions (i.e. `sum(counter_field) + median(counter_field)` as `myFormula`) will show up as a single column (`myFormula`). The wording of the message mimics the same documentation copy provided in the ES documentation. Ref: https://github.com/elastic/elasticsearch/pull/97974 Testing SO: [export.ndjson.txt](https://github.com/elastic/kibana/files/12304924/export.ndjson.txt) ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli (cherry picked from commit 4c812e3b6d1a4e477768dae04047e79bbdc940ff) --- .../datasources/form_based/form_based.tsx | 2 + .../operations/definitions/metrics.tsx | 1 - .../datasources/form_based/utils.test.tsx | 200 +++++++++++++++++- .../public/datasources/form_based/utils.tsx | 142 +++++++++++-- 4 files changed, 326 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx b/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx index 34ad0400e951dd..1a1661e7b2e195 100644 --- a/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx +++ b/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx @@ -71,6 +71,7 @@ import { isColumnInvalid, cloneLayer, getNotifiableFeatures, + getUnsupportedOperationsWarningMessage, } from './utils'; import { getUniqueLabelGenerator, isDraggedDataViewField } from '../../utils'; import { hasField, normalizeOperationDataType } from './pure_utils'; @@ -891,6 +892,7 @@ export function getFormBasedDatasource({ core.docLinks, setState ), + ...getUnsupportedOperationsWarningMessage(state, frameDatasourceAPI, core.docLinks), ]; const infoMessages = getNotifiableFeatures(state, frameDatasourceAPI, visualizationInfo); diff --git a/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/metrics.tsx b/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/metrics.tsx index deb0c19dc4837a..2b9d45979b4464 100644 --- a/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/metrics.tsx +++ b/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/metrics.tsx @@ -126,7 +126,6 @@ function buildMetricOperation>({ newField && supportedTypes.includes(newField.type) && newField.aggregatable && - isTimeSeriesCompatible(type, newField.timeSeriesMetric) && (!newField.aggregationRestrictions || newField.aggregationRestrictions![type]) ); }, diff --git a/x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx b/x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx index ca7f3460331116..f4aa785bd25f23 100644 --- a/x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx +++ b/x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx @@ -8,15 +8,20 @@ import React from 'react'; import { shallow } from 'enzyme'; import { createDatatableUtilitiesMock } from '@kbn/data-plugin/common/mocks'; -import { getPrecisionErrorWarningMessages, cloneLayer } from './utils'; +import { + getPrecisionErrorWarningMessages, + cloneLayer, + getUnsupportedOperationsWarningMessage, +} from './utils'; import type { FormBasedPrivateState, GenericIndexPatternColumn } from './types'; -import type { FramePublicAPI } from '../../types'; +import type { FramePublicAPI, IndexPattern } from '../../types'; import type { DocLinksStart } from '@kbn/core/public'; import { EuiLink } from '@elastic/eui'; import { TermsIndexPatternColumn } from './operations'; import { mountWithIntl } from '@kbn/test-jest-helpers'; import { FormattedMessage } from '@kbn/i18n-react'; import { FormBasedLayer } from './types'; +import { createMockedIndexPatternWithAdditionalFields } from './mocks'; describe('indexpattern_datasource utils', () => { describe('getPrecisionErrorWarningMessages', () => { @@ -240,4 +245,195 @@ describe('indexpattern_datasource utils', () => { ).toMatchSnapshot(); }); }); + + describe('getUnsupportedOperationsWarningMessage', () => { + let docLinks: DocLinksStart; + const affectedOperations = [ + 'sum', + 'average', + 'percentile', + 'percentile_rank', + 'count', + 'unique_count', + 'standard_deviation', + ]; + + function createColumnsForField(field: string, colOffset: number = 0) { + return Object.fromEntries( + affectedOperations.map((operationType, i) => [ + `col_${i + colOffset}`, + { operationType, sourceField: field, label: `${operationType} of ${field}` }, + ]) + ); + } + + function createState(fields: string[]) { + return { + layers: { + id: { + indexPatternId: '0', + columns: Object.assign( + {}, + ...fields.map((field, i) => + createColumnsForField(field, i * affectedOperations.length) + ) + ), + }, + }, + } as unknown as FormBasedPrivateState; + } + + function createFramePublic(indexPattern: IndexPattern): FramePublicAPI { + return { + dataViews: { + indexPatterns: Object.fromEntries([indexPattern].map((dataView, i) => [i, dataView])), + }, + } as unknown as FramePublicAPI; + } + + function createFormulaColumns(formulaParts: string[], field: string, colOffset: number = 0) { + const fullFormula = formulaParts.map((part) => `${part}(${field})`).join(' + '); + // just assume it's a sum of all the parts for testing + const rootId = `col-formula${colOffset}`; + return Object.fromEntries([ + [ + rootId, + { + operationType: 'formula', + label: `Formula: ${fullFormula}`, + params: { formula: fullFormula }, + }, + ], + ...formulaParts.map((part, i) => [ + `${rootId}X${i}`, + { operationType: part, sourceField: field, label: 'Part of formula' }, + ]), + [ + `${rootId}X${formulaParts.length}`, + { operationType: 'math', references: formulaParts.map((_, i) => `${rootId}X${i}`) }, + ], + ]); + } + + beforeEach(() => { + docLinks = { + links: { + fleet: { + datastreamsTSDSMetrics: 'http://tsdb_metric_doc', + }, + }, + } as DocLinksStart; + }); + + it.each([['bytes'], ['bytes_gauge']])( + 'should return no warning for non-counter fields: %s', + (fieldName: string) => { + const warnings = getUnsupportedOperationsWarningMessage( + createState([fieldName]), + createFramePublic( + createMockedIndexPatternWithAdditionalFields([ + { + name: 'bytes_gauge', + displayName: 'bytes_gauge', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'gauge', + }, + ]) + ), + docLinks + ); + expect(warnings).toHaveLength(0); + } + ); + + it('should return a warning for a counter field grouped by field', () => { + const warnings = getUnsupportedOperationsWarningMessage( + createState(['bytes_counter']), + createFramePublic( + createMockedIndexPatternWithAdditionalFields([ + { + name: 'bytes_counter', + displayName: 'bytes_counter', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'counter', + }, + ]) + ), + docLinks + ); + expect(warnings).toHaveLength(1); + }); + + it('should group multiple warnings by field', () => { + const warnings = getUnsupportedOperationsWarningMessage( + createState(['bytes_counter', 'bytes_counter2']), + createFramePublic( + createMockedIndexPatternWithAdditionalFields([ + { + name: 'bytes_counter', + displayName: 'bytes_counter', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'counter', + }, + { + name: 'bytes_counter2', + displayName: 'bytes_counter2', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'counter', + }, + ]) + ), + docLinks + ); + expect(warnings).toHaveLength(2); + }); + + it('should handle formula reporting only the top visible dimension', () => { + const warnings = getUnsupportedOperationsWarningMessage( + { + layers: { + id: { + indexPatternId: '0', + columns: Object.assign( + {}, + ...['bytes_counter', 'bytes_counter2'].map((field, i) => + createFormulaColumns(affectedOperations, field, i * affectedOperations.length) + ) + ), + }, + }, + } as unknown as FormBasedPrivateState, + createFramePublic( + createMockedIndexPatternWithAdditionalFields([ + { + name: 'bytes_counter', + displayName: 'bytes_counter', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'counter', + }, + { + name: 'bytes_counter2', + displayName: 'bytes_counter2', + type: 'number', + aggregatable: true, + searchable: true, + timeSeriesMetric: 'counter', + }, + ]) + ), + docLinks + ); + expect(warnings).toHaveLength(2); + }); + }); }); diff --git a/x-pack/plugins/lens/public/datasources/form_based/utils.tsx b/x-pack/plugins/lens/public/datasources/form_based/utils.tsx index 8d6720c9f744ad..6510f8563552ad 100644 --- a/x-pack/plugins/lens/public/datasources/form_based/utils.tsx +++ b/x-pack/plugins/lens/public/datasources/form_based/utils.tsx @@ -14,7 +14,7 @@ import { TimeRange } from '@kbn/es-query'; import { EuiLink, EuiSpacer, EuiText } from '@elastic/eui'; import type { DatatableColumn } from '@kbn/expressions-plugin/common'; -import { groupBy, escape, uniq } from 'lodash'; +import { groupBy, escape, uniq, uniqBy } from 'lodash'; import type { Query } from '@kbn/data-plugin/common'; import { SearchRequest } from '@kbn/data-plugin/common'; @@ -40,16 +40,19 @@ import type { ReferenceBasedIndexPatternColumn } from './operations/definitions/ import { operationDefinitionMap, - GenericIndexPatternColumn, - TermsIndexPatternColumn, - CountIndexPatternColumn, + getReferenceRoot, updateColumnParam, updateDefaultLabels, - RangeIndexPatternColumn, - FormulaIndexPatternColumn, - DateHistogramIndexPatternColumn, - MaxIndexPatternColumn, - MinIndexPatternColumn, + type GenericIndexPatternColumn, + type TermsIndexPatternColumn, + type CountIndexPatternColumn, + type RangeIndexPatternColumn, + type FormulaIndexPatternColumn, + type DateHistogramIndexPatternColumn, + type MaxIndexPatternColumn, + type MinIndexPatternColumn, + type GenericOperationDefinition, + type FieldBasedIndexPatternColumn, } from './operations'; import { getInvalidFieldMessage, isColumnOfType } from './operations/definitions/helpers'; @@ -338,6 +341,113 @@ export function getShardFailuresWarningMessages( return []; } +export function getUnsupportedOperationsWarningMessage( + state: FormBasedPrivateState, + { dataViews }: FramePublicAPI, + docLinks: DocLinksStart +) { + const warningMessages: UserMessage[] = []; + const columnsWithUnsupportedOperations: Array< + [FieldBasedIndexPatternColumn, ReferenceBasedIndexPatternColumn | undefined] + > = Object.values(state.layers) + // filter layers without dataView loaded yet + .filter(({ indexPatternId }) => dataViews.indexPatterns[indexPatternId]) + .flatMap((layer) => { + const dataView = dataViews.indexPatterns[layer.indexPatternId]; + const columnsEntries = Object.entries(layer.columns); + return columnsEntries + .filter(([_, column]) => { + if (!hasField(column)) { + return false; + } + const field = dataView.getFieldByName(column.sourceField); + if (!field) { + return false; + } + return ( + !( + operationDefinitionMap[column.operationType] as Extract< + GenericOperationDefinition, + { input: 'field' } + > + ).getPossibleOperationForField(field) && field?.timeSeriesMetric === 'counter' + ); + }) + .map( + ([id, fieldColumn]) => + [fieldColumn, layer.columns[getReferenceRoot(layer, id)]] as [ + FieldBasedIndexPatternColumn, + ReferenceBasedIndexPatternColumn | undefined + ] + ); + }); + if (columnsWithUnsupportedOperations.length) { + // group the columns by field + // then group together columns of a formula/referenced operation who use the same field + const columnsGroupedByField = Object.values( + groupBy(columnsWithUnsupportedOperations, ([column]) => column.sourceField) + ).map((columnsList) => uniqBy(columnsList, ([column, rootColumn]) => rootColumn ?? column)); + + for (const columnsGrouped of columnsGroupedByField) { + const sourceField = columnsGrouped[0][0].sourceField; + warningMessages.push({ + severity: 'warning', + fixableInEditor: false, + displayLocations: [{ id: 'toolbar' }, { id: 'embeddableBadge' }], + shortMessage: i18n.translate( + 'xpack.lens.indexPattern.tsdbErrorWarning.unsupportedCounterOperationErrorWarning.shortMessage', + { + defaultMessage: + 'The result of {count} {count, plural, one {operation} other {operations}} might be meaningless for {field}: {operations}', + values: { + count: columnsGrouped.length, + operations: columnsGrouped + .map(([affectedColumn, rootColumn]) => (rootColumn ?? affectedColumn).label) + .join(', '), + field: sourceField, + }, + } + ), + longMessage: ( + <> + + {columnsGrouped.map(([affectedColumn, rootColumn], i) => ( + + {(rootColumn ?? affectedColumn).label} + {i < columnsGrouped.length - 1 ? ', ' : ''} + + ))} + + ), + field: sourceField, + link: ( + + + + ), + }} + /> + + ), + }); + } + } + return warningMessages; +} + export function getPrecisionErrorWarningMessages( datatableUtilities: DatatableUtilitiesService, state: FormBasedPrivateState, @@ -349,18 +459,18 @@ export function getPrecisionErrorWarningMessages( if (state && activeData) { Object.entries(activeData) - .reduce( - (acc, [layerId, { columns }]) => [ - ...acc, - ...columns.map((column) => ({ layerId, column })), - ], - [] as Array<{ layerId: string; column: DatatableColumn }> - ) + .reduce((acc, [layerId, { columns }]) => { + acc.push(...columns.map((column) => ({ layerId, column }))); + return acc; + }, [] as Array<{ layerId: string; column: DatatableColumn }>) .forEach(({ layerId, column }) => { const currentLayer = state.layers[layerId]; const currentColumn = currentLayer?.columns[column.id]; if (currentLayer && currentColumn && datatableUtilities.hasPrecisionError(column)) { const indexPattern = dataViews.indexPatterns[currentLayer.indexPatternId]; + if (!indexPattern) { + return; + } // currentColumnIsTerms is mostly a type guard. If there's a precision error, // we already know that we're dealing with a terms-based operation (at least for now). const currentColumnIsTerms = isColumnOfType(