From 0a25b39df25b61537c7c5aec9c441195f99845fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cau=C3=AA=20Marcondes?= <55978943+cauemarcondes@users.noreply.github.com> Date: Tue, 24 Oct 2023 13:53:14 +0100 Subject: [PATCH] [Profiling] Fix Diff TopN bug (#169549) Fixing this error that was happening when hovering over the grid. Screenshot 2023-10-23 at 17 14 26 --- .../observability/server/ui_settings.ts | 6 +- .../e2e/profiling_views/functions.cy.ts | 4 + .../topn_functions/function_row.tsx | 100 ++++++++++++------ 3 files changed, 73 insertions(+), 37 deletions(-) diff --git a/x-pack/plugins/observability/server/ui_settings.ts b/x-pack/plugins/observability/server/ui_settings.ts index 134ad77046dbc..1facce3f09984 100644 --- a/x-pack/plugins/observability/server/ui_settings.ts +++ b/x-pack/plugins/observability/server/ui_settings.ts @@ -396,7 +396,7 @@ export const uiSettings: Record = { defaultMessage: `The average amortized per-core power consumption (based on 100% CPU utilization).`, }), schema: schema.number({ min: 0 }), - requiresPageReload: false, + requiresPageReload: true, }, [profilingDatacenterPUE]: { category: [observabilityFeatureId], @@ -425,7 +425,7 @@ export const uiSettings: Record = { }, }), schema: schema.number({ min: 0 }), - requiresPageReload: false, + requiresPageReload: true, }, [profilingCo2PerKWH]: { category: [observabilityFeatureId], @@ -448,7 +448,7 @@ export const uiSettings: Record = { }, }), schema: schema.number({ min: 0 }), - requiresPageReload: false, + requiresPageReload: true, }, }; diff --git a/x-pack/plugins/profiling/e2e/cypress/e2e/profiling_views/functions.cy.ts b/x-pack/plugins/profiling/e2e/cypress/e2e/profiling_views/functions.cy.ts index ca972d0605e22..ac678e4650d02 100644 --- a/x-pack/plugins/profiling/e2e/cypress/e2e/profiling_views/functions.cy.ts +++ b/x-pack/plugins/profiling/e2e/cypress/e2e/profiling_views/functions.cy.ts @@ -203,6 +203,10 @@ describe.skip('Functions page', () => { .clear() .type('20'); cy.contains('Save changes').click(); + cy.getByTestSubj('kbnLoadingMessage').should('exist'); + cy.getByTestSubj('kbnLoadingMessage').should('not.exist', { + timeout: 50000, + }); cy.go('back'); cy.wait('@getTopNFunctions'); cy.get(firstRowSelector).eq(5).contains('24.22k lbs / 10.99k'); diff --git a/x-pack/plugins/profiling/public/components/topn_functions/function_row.tsx b/x-pack/plugins/profiling/public/components/topn_functions/function_row.tsx index 0099cbc82ffec..6cf0159dcdc01 100644 --- a/x-pack/plugins/profiling/public/components/topn_functions/function_row.tsx +++ b/x-pack/plugins/profiling/public/components/topn_functions/function_row.tsx @@ -15,8 +15,8 @@ import { useEuiTheme, } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; -import React from 'react'; import { TopNFunctionSortField } from '@kbn/profiling-utils'; +import React, { useEffect } from 'react'; import { asCost } from '../../utils/formatters/as_cost'; import { asWeight } from '../../utils/formatters/as_weight'; import { StackFrameSummary } from '../stack_frame_summary'; @@ -39,38 +39,8 @@ export function FunctionRow({ onFrameClick, setCellProps, }: Props) { - const theme = useEuiTheme(); - const successColor = useEuiBackgroundColor('success'); - const dangerColor = useEuiBackgroundColor('danger'); - if (columnId === TopNFunctionSortField.Diff) { - if (!functionRow.diff) { - return ( - - {i18n.translate('xpack.profiling.functionsView.newLabel', { - defaultMessage: 'New', - })} - - ); - } - - if (functionRow.diff.rank === 0) { - return null; - } - - const color = functionRow.diff.rank > 0 ? 'success' : 'danger'; - setCellProps({ style: { backgroundColor: color === 'success' ? successColor : dangerColor } }); - - return ( - - - 0 ? 'sortUp' : 'sortDown'} color={color} /> - - - {Math.abs(functionRow.diff.rank)} - - - ); + return ; } if (columnId === TopNFunctionSortField.Rank) { @@ -82,12 +52,12 @@ export function FunctionRow({ } if (columnId === TopNFunctionSortField.Samples) { - setCellProps({ css: { textAlign: 'right' } }); return ( - ); } @@ -116,3 +86,65 @@ export function FunctionRow({ return null; } + +interface SamplesColumnProps { + samples: number; + diffSamples?: number; + totalSamples: number; + setCellProps: EuiDataGridCellValueElementProps['setCellProps']; +} + +function SamplesColumn({ samples, totalSamples, diffSamples, setCellProps }: SamplesColumnProps) { + useEffect(() => { + setCellProps({ css: { textAlign: 'right' } }); + }, [setCellProps]); + return ; +} + +interface DiffColumnProps { + diff: IFunctionRow['diff']; + setCellProps: EuiDataGridCellValueElementProps['setCellProps']; +} + +function DiffColumn({ diff, setCellProps }: DiffColumnProps) { + const theme = useEuiTheme(); + const successColor = useEuiBackgroundColor('success'); + const dangerColor = useEuiBackgroundColor('danger'); + + useEffect(() => { + if (diff && diff.rank > 0) { + const color = diff.rank > 0 ? 'success' : 'danger'; + setCellProps({ + style: { backgroundColor: color === 'success' ? successColor : dangerColor }, + }); + } + }, [dangerColor, diff, setCellProps, successColor]); + + if (!diff) { + return ( + + {i18n.translate('xpack.profiling.functionsView.newLabel', { + defaultMessage: 'New', + })} + + ); + } + + if (diff.rank === 0) { + return null; + } + + return ( + + + 0 ? 'sortUp' : 'sortDown'} + color={diff.rank > 0 ? 'success' : 'danger'} + /> + + + {Math.abs(diff.rank)} + + + ); +}