From e2c6bffb47375695d1e77f75930b04296d78b18c Mon Sep 17 00:00:00 2001 From: Dima Arnautov Date: Fri, 27 Oct 2023 17:13:52 +0200 Subject: [PATCH] [ML] AIOps: Fix Change point embeddable reporting (#169962) ## Summary Fixes #169733 #### Reporting fix Change point detection embeddable was incorrectly reporting render completion. It was relying on the `onLoad` callback from the Lens embeddable responsible for chart rendering, which only indicates that data fetching is complete, but not the actual rendering. Current implementation relies on the `renderComplete` event from each child embeddable. Both PNG and PDF exports tested and work as expected. ![DASHBOARDDDD](https://github.com/elastic/kibana/assets/5236598/fb718f31-5862-43ab-82e3-60ebb795b8eb) #### Additional fixes - Fixes the metric and split field controls states when editing existing Change point embeddable from a dashboard - Fixes `filter` query if partitions input is initialized as an empty array. ### Checklist - [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 (cherry picked from commit 80d382a22f2adc39a63146d3ffb5cb7763090c2e) # Conflicts: # .eslintrc.js --- .../chart_component.tsx | 62 ++++++--- .../change_point_detection/charts_grid.tsx | 13 +- .../change_point_detection/fields_config.tsx | 8 +- .../partitions_selector.tsx | 4 +- .../change_point_chart_initializer.tsx | 2 +- .../embeddable_change_point_chart.tsx | 3 + .../embeddable_chart_component_wrapper.tsx | 2 +- .../apps/aiops/change_point_detection.ts | 8 ++ .../aiops/change_point_detection_page.ts | 128 ++++++++++++++++++ 9 files changed, 205 insertions(+), 25 deletions(-) diff --git a/x-pack/plugins/aiops/public/components/change_point_detection/chart_component.tsx b/x-pack/plugins/aiops/public/components/change_point_detection/chart_component.tsx index 0fb6a675c2bdc5..1390bf82b1e5f2 100644 --- a/x-pack/plugins/aiops/public/components/change_point_detection/chart_component.tsx +++ b/x-pack/plugins/aiops/public/components/change_point_detection/chart_component.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React, { FC } from 'react'; +import React, { FC, useCallback, useEffect, useRef } from 'react'; import type { Filter, Query, TimeRange } from '@kbn/es-query'; import { useCommonChartProps } from './use_common_chart_props'; import { useAiopsAppContext } from '../../hooks/use_aiops_app_context'; @@ -18,6 +18,7 @@ export interface ChartComponentProps { interval: string; onLoading?: (isLoading: boolean) => void; + onRenderComplete?: () => void; } export interface ChartComponentPropsAll { @@ -31,11 +32,34 @@ export interface ChartComponentPropsAll { } export const ChartComponent: FC = React.memo( - ({ annotation, fieldConfig, interval, onLoading }) => { + ({ annotation, fieldConfig, interval, onLoading, onRenderComplete }) => { const { lens: { EmbeddableComponent }, } = useAiopsAppContext(); + const chartWrapperRef = useRef(null); + + const renderCompleteListener = useCallback( + (event: Event) => { + if (event.target === chartWrapperRef.current) return; + if (onRenderComplete) { + onRenderComplete(); + } + }, + [onRenderComplete] + ); + + useEffect(() => { + if (!chartWrapperRef.current) { + throw new Error('Reference to the chart wrapper is not set'); + } + const chartWrapper = chartWrapperRef.current; + chartWrapper.addEventListener('renderComplete', renderCompleteListener); + return () => { + chartWrapper.removeEventListener('renderComplete', renderCompleteListener); + }; + }, [renderCompleteListener]); + const { filters, timeRange, query, attributes } = useCommonChartProps({ fieldConfig, annotation, @@ -43,22 +67,24 @@ export const ChartComponent: FC = React.memo( }); return ( - +
+ +
); } ); diff --git a/x-pack/plugins/aiops/public/components/change_point_detection/charts_grid.tsx b/x-pack/plugins/aiops/public/components/change_point_detection/charts_grid.tsx index 6cbda6c9ced9b5..f5ebc0783213d3 100644 --- a/x-pack/plugins/aiops/public/components/change_point_detection/charts_grid.tsx +++ b/x-pack/plugins/aiops/public/components/change_point_detection/charts_grid.tsx @@ -52,7 +52,11 @@ export const ChartsGrid: FC<{ Object.fromEntries(changePoints.map((v, i) => [i, true])) ); - const onLoadCallback = useCallback( + /** + * Callback to track render of each chart component + * to report when all charts are ready. + */ + const onChartRenderCompleteCallback = useCallback( (chartId: number, isLoading: boolean) => { if (!onRenderComplete) return; loadCounter.current[chartId] = isLoading; @@ -141,7 +145,12 @@ export const ChartsGrid: FC<{ annotation={v} interval={interval} onLoading={(isLoading) => { - onLoadCallback(index, isLoading); + if (isLoading) { + onChartRenderCompleteCallback(index, true); + } + }} + onRenderComplete={() => { + onChartRenderCompleteCallback(index, false); }} /> diff --git a/x-pack/plugins/aiops/public/components/change_point_detection/fields_config.tsx b/x-pack/plugins/aiops/public/components/change_point_detection/fields_config.tsx index c07af22a5f16a5..0776bbba908a62 100644 --- a/x-pack/plugins/aiops/public/components/change_point_detection/fields_config.tsx +++ b/x-pack/plugins/aiops/public/components/change_point_detection/fields_config.tsx @@ -236,6 +236,7 @@ const FieldPanel: FC = ({ }), icon: 'plusInCircle', panel: 'attachMainPanel', + 'data-test-subj': 'aiopsChangePointDetectionAttachButton', }, ] : []), @@ -248,6 +249,7 @@ const FieldPanel: FC = ({ disabled: removeDisabled, }, ], + 'data=test-subj': 'aiopsChangePointDetectionContextMenuPanel', }, { id: 'attachMainPanel', @@ -269,6 +271,7 @@ const FieldPanel: FC = ({ defaultMessage: 'To dashboard', }), panel: 'attachToDashboardPanel', + 'data-test-subj': 'aiopsChangePointDetectionAttachToDashboardButton', }, ] : []), @@ -290,6 +293,7 @@ const FieldPanel: FC = ({ ), } : {}), + 'data-test-subj': 'aiopsChangePointDetectionAttachToCaseButton', onClick: () => { openCasesModalCallback({ timeRange, @@ -308,6 +312,7 @@ const FieldPanel: FC = ({ ] : []), ], + 'data-test-subj': 'aiopsChangePointDetectionAttachChartPanel', }, { id: 'attachToDashboardPanel', @@ -318,7 +323,7 @@ const FieldPanel: FC = ({ content: ( - + = ({ }) } compressed + data-test-subj="aiopsChangePointDetectionAttachToDashboardApplyTimeRangeSwitch" /> {isDefined(fieldConfig.splitField) && selectedPartitions.length === 0 ? ( diff --git a/x-pack/plugins/aiops/public/components/change_point_detection/partitions_selector.tsx b/x-pack/plugins/aiops/public/components/change_point_detection/partitions_selector.tsx index 6e040c7d77e621..4363de30ce162a 100644 --- a/x-pack/plugins/aiops/public/components/change_point_detection/partitions_selector.tsx +++ b/x-pack/plugins/aiops/public/components/change_point_detection/partitions_selector.tsx @@ -140,8 +140,8 @@ export const PartitionsSelector: FC = ({ useEffect( function onSplitFieldChange() { - if (splitField !== prevSplitField) { - fetchResults(''); + fetchResults(''); + if (prevSplitField !== undefined && splitField !== prevSplitField) { onChange([]); } }, diff --git a/x-pack/plugins/aiops/public/embeddable/change_point_chart_initializer.tsx b/x-pack/plugins/aiops/public/embeddable/change_point_chart_initializer.tsx index 50488c06cec7cf..71780f26a4fcb5 100644 --- a/x-pack/plugins/aiops/public/embeddable/change_point_chart_initializer.tsx +++ b/x-pack/plugins/aiops/public/embeddable/change_point_chart_initializer.tsx @@ -205,7 +205,7 @@ export const FormControls: FC<{ return; } - if (metricFieldOptions === prevMetricFieldOptions) return; + if (!prevMetricFieldOptions || metricFieldOptions === prevMetricFieldOptions) return; onChange({ fn: formInput.fn, diff --git a/x-pack/plugins/aiops/public/embeddable/embeddable_change_point_chart.tsx b/x-pack/plugins/aiops/public/embeddable/embeddable_change_point_chart.tsx index af5942024ec99d..ff577b0cc281c5 100644 --- a/x-pack/plugins/aiops/public/embeddable/embeddable_change_point_chart.tsx +++ b/x-pack/plugins/aiops/public/embeddable/embeddable_change_point_chart.tsx @@ -111,6 +111,9 @@ export class EmbeddableChangePointChart extends AbstractEmbeddable< // required for the export feature to work this.node.setAttribute('data-shared-item', ''); + // test subject selector for functional tests + this.node.setAttribute('data-test-subj', 'aiopsEmbeddableChangePointChart'); + const I18nContext = this.deps.i18n.Context; const datePickerDeps = { diff --git a/x-pack/plugins/aiops/public/embeddable/embeddable_chart_component_wrapper.tsx b/x-pack/plugins/aiops/public/embeddable/embeddable_chart_component_wrapper.tsx index 43fbe2ff926062..cbfff714d66fb1 100644 --- a/x-pack/plugins/aiops/public/embeddable/embeddable_chart_component_wrapper.tsx +++ b/x-pack/plugins/aiops/public/embeddable/embeddable_chart_component_wrapper.tsx @@ -170,7 +170,7 @@ export const ChartGridEmbeddableWrapper: FC< }, }); - if (partitions && fieldConfig.splitField) { + if (Array.isArray(partitions) && partitions.length > 0 && fieldConfig.splitField) { mergedQuery.bool?.filter.push({ terms: { [fieldConfig.splitField]: partitions, diff --git a/x-pack/test/functional/apps/aiops/change_point_detection.ts b/x-pack/test/functional/apps/aiops/change_point_detection.ts index 0cbff0642aa3c0..f643de514c0cb1 100644 --- a/x-pack/test/functional/apps/aiops/change_point_detection.ts +++ b/x-pack/test/functional/apps/aiops/change_point_detection.ts @@ -94,5 +94,13 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { await aiops.changePointDetectionPage.addChangePointConfig(); await aiops.changePointDetectionPage.assertPanelExist(1); }); + + it('attaches change point charts to a dashboard', async () => { + await aiops.changePointDetectionPage.assertPanelExist(0); + await aiops.changePointDetectionPage.attachChartsToDashboard(0, { + applyTimeRange: true, + maxSeries: 1, + }); + }); }); } diff --git a/x-pack/test/functional/services/aiops/change_point_detection_page.ts b/x-pack/test/functional/services/aiops/change_point_detection_page.ts index 2eb539ef4fc784..4f83137df472e4 100644 --- a/x-pack/test/functional/services/aiops/change_point_detection_page.ts +++ b/x-pack/test/functional/services/aiops/change_point_detection_page.ts @@ -9,6 +9,11 @@ import expect from '@kbn/expect'; import { FtrProviderContext } from '../../ftr_provider_context'; import { MlTableService } from '../ml/common_table_service'; +export interface DashboardAttachmentOptions { + applyTimeRange: boolean; + maxSeries: number; +} + export function ChangePointDetectionPageProvider( { getService, getPageObject }: FtrProviderContext, tableService: MlTableService @@ -18,6 +23,7 @@ export function ChangePointDetectionPageProvider( const comboBox = getService('comboBox'); const browser = getService('browser'); const elasticChart = getService('elasticChart'); + const dashboardPage = getPageObject('dashboard'); return { async navigateToIndexPatternSelection() { @@ -124,6 +130,128 @@ export function ChangePointDetectionPageProvider( }); }, + async openPanelContextMenu(panelIndex: number) { + await testSubjects.click( + `aiopsChangePointPanel_${panelIndex} > aiopsChangePointDetectionContextMenuButton` + ); + await retry.tryForTime(30 * 1000, async () => { + await testSubjects.existOrFail(`aiopsChangePointDetectionAttachButton`); + }); + }, + + async clickAttachChartsButton() { + await testSubjects.click('aiopsChangePointDetectionAttachButton'); + await retry.tryForTime(30 * 1000, async () => { + await testSubjects.missingOrFail(`aiopsChangePointDetectionAttachButton`); + await testSubjects.existOrFail(`aiopsChangePointDetectionAttachToDashboardButton`); + }); + }, + + async clickAttachDashboardButton() { + await testSubjects.click('aiopsChangePointDetectionAttachToDashboardButton'); + await retry.tryForTime(30 * 1000, async () => { + await testSubjects.existOrFail(`aiopsChangePointDetectionDashboardAttachmentForm`); + }); + }, + + async assertApplyTimeRangeControl(expectedValue: boolean) { + const isChecked = await testSubjects.isEuiSwitchChecked( + `aiopsChangePointDetectionAttachToDashboardApplyTimeRangeSwitch` + ); + expect(isChecked).to.eql( + expectedValue, + `Expected apply time range to be ${expectedValue ? 'enabled' : 'disabled'}` + ); + }, + + async assertMaxSeriesControl(expectedValue: number) { + const currentValue = Number( + await testSubjects.getAttribute('aiopsMaxSeriesControlFieldNumber', 'value') + ); + expect(currentValue).to.eql( + expectedValue, + `Expected max series control to be ${expectedValue} (got ${currentValue})` + ); + }, + + async toggleApplyTimeRangeControl(isChecked: boolean) { + await testSubjects.setEuiSwitch( + `aiopsChangePointDetectionAttachToDashboardApplyTimeRangeSwitch`, + isChecked ? 'check' : 'uncheck' + ); + await this.assertApplyTimeRangeControl(isChecked); + }, + + async setMaxSeriesControl(value: number) { + await testSubjects.setValue('aiopsMaxSeriesControlFieldNumber', value.toString()); + await this.assertMaxSeriesControl(value); + }, + + async completeDashboardAttachmentForm(attachmentOptions: DashboardAttachmentOptions) { + // assert default values + await this.assertApplyTimeRangeControl(false); + await this.assertMaxSeriesControl(6); + + if (attachmentOptions.applyTimeRange) { + await this.toggleApplyTimeRangeControl(attachmentOptions.applyTimeRange); + } + + if (attachmentOptions.maxSeries) { + await this.setMaxSeriesControl(attachmentOptions.maxSeries); + } + + await testSubjects.click('aiopsChangePointDetectionSubmitDashboardAttachButton'); + + await retry.tryForTime(30 * 1000, async () => { + // await testSubjects.missingOrFail(`aiopsChangePointDetectionSubmitDashboardAttachButton`); + await testSubjects.existOrFail('savedObjectSaveModal'); + }); + }, + + async completeSaveToDashboardForm(options?: { createNew: boolean; dashboardName?: string }) { + await retry.tryForTime(30 * 1000, async () => { + const dashboardSelector = await testSubjects.find('add-to-dashboard-options'); + + if (options?.createNew) { + const label = await dashboardSelector.findByCssSelector( + `label[for="new-dashboard-option"]` + ); + await label.click(); + } + + await testSubjects.click('confirmSaveSavedObjectButton'); + await retry.waitForWithTimeout('Save modal to disappear', 1000, () => + testSubjects + .missingOrFail('confirmSaveSavedObjectButton') + .then(() => true) + .catch(() => false) + ); + + // make sure the dashboard page actually loaded + const dashboardItemCount = await dashboardPage.getSharedItemsCount(); + expect(dashboardItemCount).to.not.eql(undefined); + }); + // changing to the dashboard app might take some time + const embeddable = await testSubjects.find('aiopsEmbeddableChangePointChart', 30 * 1000); + const lensChart = await embeddable.findByClassName('lnsExpressionRenderer'); + expect(await lensChart.isDisplayed()).to.eql( + true, + 'Change point detection chart should be displayed in dashboard' + ); + }, + + async attachChartsToDashboard( + panelIndex: number, + attachmentOptions: DashboardAttachmentOptions + ) { + await this.assertPanelExist(panelIndex); + await this.openPanelContextMenu(panelIndex); + await this.clickAttachChartsButton(); + await this.clickAttachDashboardButton(); + await this.completeDashboardAttachmentForm(attachmentOptions); + await this.completeSaveToDashboardForm({ createNew: true }); + }, + getTable(index: number) { return tableService.getServiceInstance( 'ChangePointResultsTable',