From 032631bca61bf9121c586700e4cf128e73711d69 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 18 Jul 2024 21:54:19 +0200 Subject: [PATCH 1/5] Switch to ratios for slow and frozen frames --- static/app/components/gridEditable/index.tsx | 1 + .../components/tables/screensTable.tsx | 8 +++ .../common/components/tables/screensTable.tsx | 31 ++++++++++ .../tables/spanOperationTable.spec.tsx | 8 +-- .../components/tables/spanOperationTable.tsx | 59 ++++++++++++------- .../ui/components/tables/uiScreensTable.tsx | 59 +++++++++++++------ .../mobile/ui/components/uiScreens.spec.tsx | 16 ++--- .../mobile/ui/components/uiScreens.tsx | 8 +-- 8 files changed, 136 insertions(+), 54 deletions(-) diff --git a/static/app/components/gridEditable/index.tsx b/static/app/components/gridEditable/index.tsx index e301366dd19a62..9cc9203b69c578 100644 --- a/static/app/components/gridEditable/index.tsx +++ b/static/app/components/gridEditable/index.tsx @@ -45,6 +45,7 @@ export type GridColumn = { export type GridColumnHeader = GridColumn & { name: string; + toolTip?: string; }; export type GridColumnOrder = GridColumnHeader; diff --git a/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx b/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx index 5f3b0b6e2ac6e0..bcab565d0c55ec 100644 --- a/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx +++ b/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx @@ -64,6 +64,13 @@ export function AppStartScreens({data, eventView, isLoading, pageLinks}: Props) 'count_starts(measurements.app_start_warm)': t('Warm Start Count'), }; + const columnToolTipMap = { + [`avg_compare(measurements.app_start_cold,release,${primaryRelease},${secondaryRelease})`]: + t('Average Cold Start difference'), + [`avg_compare(measurements.app_start_warm,release,${primaryRelease},${secondaryRelease})`]: + t('Average Warm Start difference'), + }; + function renderBodyCell(column, row): React.ReactNode { if (!data) { return null; @@ -121,6 +128,7 @@ export function AppStartScreens({data, eventView, isLoading, pageLinks}: Props) return ( ; columnOrder: string[]; + columnToolTipMap: Record | undefined; data: TableData | undefined; defaultSort: GridColumnSortBy[]; eventView: EventView; @@ -42,6 +45,7 @@ export function ScreensTable({ pageLinks, columnNameMap, columnOrder, + columnToolTipMap, defaultSort, customBodyCellRenderer, moduleName, @@ -116,6 +120,20 @@ export function ScreensTable({ generateSortLink={generateSortLink} /> ); + + function columnWithToolTip(tooltipTitle: string) { + return ( + + {tooltipTitle}}> + {sortLink} + + + ); + } + + if (column.toolTip) { + return columnWithToolTip(column.toolTip); + } return sortLink; } @@ -129,6 +147,7 @@ export function ScreensTable({ key: columnKey, name: columnNameMap[columnKey], width: COL_WIDTH_UNDEFINED, + toolTip: columnToolTipMap ? columnToolTipMap[columnKey] : undefined, }; })} columnSortBy={defaultSort} @@ -152,3 +171,15 @@ export function ScreensTable({ ); } + +const Alignment = styled('span')<{align: string}>` + display: block; + margin: auto; + text-align: ${props => props.align}; + width: 100%; +`; + +const StyledTooltip = styled(Tooltip)` + top: 1px; + position: relative; +`; diff --git a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.spec.tsx b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.spec.tsx index 0127bdf0c0bd66..cb42a8d8e7b0b3 100644 --- a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.spec.tsx +++ b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.spec.tsx @@ -63,10 +63,10 @@ describe('SpanOperationTable', () => { 'span.op', 'span.group', 'span.description', - 'avg_if(mobile.slow_frames,release,foo)', - 'avg_if(mobile.slow_frames,release,bar)', - 'avg_if(mobile.frozen_frames,release,foo)', - 'avg_if(mobile.frozen_frames,release,bar)', + 'division_if(mobile.slow_frames,mobile.total_frames,release,foo)', + 'division_if(mobile.slow_frames,mobile.total_frames,release,bar)', + 'division_if(mobile.frozen_frames,mobile.total_frames,release,foo)', + 'division_if(mobile.frozen_frames,mobile.total_frames,release,bar)', 'avg_if(mobile.frames_delay,release,foo)', 'avg_if(mobile.frames_delay,release,bar)', 'avg_compare(mobile.frames_delay,release,foo,bar)', diff --git a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx index 4ea2d297d0366a..608b4642477848 100644 --- a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx +++ b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx @@ -69,10 +69,10 @@ export function SpanOperationTable({ SPAN_OP, SPAN_GROUP, SPAN_DESCRIPTION, - `avg_if(mobile.slow_frames,release,${primaryRelease})`, - `avg_if(mobile.slow_frames,release,${secondaryRelease})`, - `avg_if(mobile.frozen_frames,release,${primaryRelease})`, - `avg_if(mobile.frozen_frames,release,${secondaryRelease})`, + `division_if(mobile.slow_frames,mobile.total_frames,release,${primaryRelease})`, + `division_if(mobile.slow_frames,mobile.total_frames,release,${secondaryRelease})`, + `division_if(mobile.frozen_frames,mobile.total_frames,release,${primaryRelease})`, + `division_if(mobile.frozen_frames,mobile.total_frames,release,${secondaryRelease})`, `avg_if(mobile.frames_delay,release,${primaryRelease})`, `avg_if(mobile.frames_delay,release,${secondaryRelease})`, `avg_compare(mobile.frames_delay,release,${primaryRelease},${secondaryRelease})`, @@ -97,22 +97,16 @@ export function SpanOperationTable({ const columnNameMap = { [SPAN_OP]: t('Operation'), [SPAN_DESCRIPTION]: t('Span Description'), - [`avg_if(mobile.slow_frames,release,${primaryRelease})`]: t( + [`division_if(mobile.slow_frames,mobile.total_frames,release,${primaryRelease})`]: t( 'Slow (%s)', PRIMARY_RELEASE_ALIAS ), - [`avg_if(mobile.slow_frames,release,${secondaryRelease})`]: t( - 'Slow (%s)', - SECONDARY_RELEASE_ALIAS - ), - [`avg_if(mobile.frozen_frames,release,${primaryRelease})`]: t( - 'Frozen (%s)', - PRIMARY_RELEASE_ALIAS - ), - [`avg_if(mobile.frozen_frames,release,${secondaryRelease})`]: t( - 'Frozen (%s)', - SECONDARY_RELEASE_ALIAS - ), + [`division_if(mobile.slow_frames,mobile.total_frames,release,${secondaryRelease})`]: + t('Slow (%s)', SECONDARY_RELEASE_ALIAS), + [`division_if(mobile.frozen_frames,mobile.total_frames,release,${primaryRelease})`]: + t('Frozen (%s)', PRIMARY_RELEASE_ALIAS), + [`division_if(mobile.frozen_frames,mobile.total_frames,release,${secondaryRelease})`]: + t('Frozen (%s)', SECONDARY_RELEASE_ALIAS), [`avg_if(mobile.frames_delay,release,${primaryRelease})`]: t( 'Delay (%s)', PRIMARY_RELEASE_ALIAS @@ -125,6 +119,28 @@ export function SpanOperationTable({ t('Change'), }; + const columnToolTipMap = { + [`division_if(mobile.slow_frames,mobile.total_frames,release,${primaryRelease})`]: t( + 'The number of slow frames divided by total frames (%s)', + PRIMARY_RELEASE_ALIAS + ), + [`division_if(mobile.slow_frames,mobile.total_frames,release,${secondaryRelease})`]: + t( + 'The number of slow frames divided by total frames (%s)', + SECONDARY_RELEASE_ALIAS + ), + [`division_if(mobile.frozen_frames,mobile.total_frames,release,${primaryRelease})`]: + t( + 'The number of frozen frames divided by total frames (%s)', + PRIMARY_RELEASE_ALIAS + ), + [`division_if(mobile.frozen_frames,mobile.total_frames,release,${secondaryRelease})`]: + t( + 'The number of frozen frames divided by total frames (%s)', + SECONDARY_RELEASE_ALIAS + ), + }; + function renderBodyCell(column, row) { if (column.key === SPAN_DESCRIPTION) { const label = row[SpanMetricsField.SPAN_DESCRIPTION]; @@ -163,6 +179,7 @@ export function SpanOperationTable({ return ( { field: [ 'project.id', 'transaction', - 'avg_if(mobile.slow_frames,release,com.example.vu.android@2.10.5)', - 'avg_if(mobile.slow_frames,release,com.example.vu.android@2.10.3+42)', - 'avg_if(mobile.frozen_frames,release,com.example.vu.android@2.10.5)', - 'avg_if(mobile.frozen_frames,release,com.example.vu.android@2.10.3+42)', + 'division_if(mobile.slow_frames,mobile.total_frames,release,com.example.vu.android@2.10.5)', + 'division_if(mobile.slow_frames,mobile.total_frames,release,com.example.vu.android@2.10.3+42)', + 'division_if(mobile.frozen_frames,mobile.total_frames,release,com.example.vu.android@2.10.5)', + 'division_if(mobile.frozen_frames,mobile.total_frames,release,com.example.vu.android@2.10.3+42)', 'avg_if(mobile.frames_delay,release,com.example.vu.android@2.10.5)', 'avg_if(mobile.frames_delay,release,com.example.vu.android@2.10.3+42)', 'avg_compare(mobile.frames_delay,release,com.example.vu.android@2.10.5,com.example.vu.android@2.10.3+42)', diff --git a/static/app/views/insights/mobile/ui/components/uiScreens.tsx b/static/app/views/insights/mobile/ui/components/uiScreens.tsx index ae0efa52a1fb3b..9bc0fe837c3599 100644 --- a/static/app/views/insights/mobile/ui/components/uiScreens.tsx +++ b/static/app/views/insights/mobile/ui/components/uiScreens.tsx @@ -66,10 +66,10 @@ export function UIScreens() { fields: [ SpanMetricsField.PROJECT_ID, 'transaction', - `avg_if(mobile.slow_frames,release,${primaryRelease})`, - `avg_if(mobile.slow_frames,release,${secondaryRelease})`, - `avg_if(mobile.frozen_frames,release,${primaryRelease})`, - `avg_if(mobile.frozen_frames,release,${secondaryRelease})`, + `division_if(mobile.slow_frames,mobile.total_frames,release,${primaryRelease})`, + `division_if(mobile.slow_frames,mobile.total_frames,release,${secondaryRelease})`, + `division_if(mobile.frozen_frames,mobile.total_frames,release,${primaryRelease})`, + `division_if(mobile.frozen_frames,mobile.total_frames,release,${secondaryRelease})`, `avg_if(mobile.frames_delay,release,${primaryRelease})`, `avg_if(mobile.frames_delay,release,${secondaryRelease})`, `avg_compare(mobile.frames_delay,release,${primaryRelease},${secondaryRelease})`, From 024fb192d558e31755cb8aa4eef5abbc3d296632 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 19 Jul 2024 09:11:48 +0200 Subject: [PATCH 2/5] Fix spec --- .../mobile/common/components/tables/screensTable.spec.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx b/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx index 72b063cff9d521..174b36cc0893ae 100644 --- a/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx +++ b/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx @@ -34,6 +34,7 @@ describe('ScreensTable', () => { columnNameMap={{ transaction: 'Screen', }} + columnToolTipMap={{}} columnOrder={['transaction']} data={{ data: [{id: '1', transaction: 'Screen 1'}], @@ -56,6 +57,7 @@ describe('ScreensTable', () => { columnNameMap={{ transaction: 'Screen', }} + columnToolTipMap={{}} columnOrder={['transaction', 'non-custom']} data={{ data: [ From 3f8720d78b195b26a3eebc0334bb8bf6e6f1fda8 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 19 Jul 2024 17:54:17 +0200 Subject: [PATCH 3/5] Make division_if sortable --- static/app/utils/discover/fieldRenderers.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/static/app/utils/discover/fieldRenderers.tsx b/static/app/utils/discover/fieldRenderers.tsx index 7ab9d686f0d33e..d75c91dc9717af 100644 --- a/static/app/utils/discover/fieldRenderers.tsx +++ b/static/app/utils/discover/fieldRenderers.tsx @@ -819,6 +819,10 @@ export function getSortField( return field; } + if (field.startsWith('division_if(')) { + return field; + } + for (const alias in AGGREGATIONS) { if (field.startsWith(alias)) { return AGGREGATIONS[alias].isSortable ? field : null; From c914283c0601d13e5c68194a204c6cd52099b52a Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 19 Jul 2024 18:13:24 +0200 Subject: [PATCH 4/5] Rename toolTip -> tooltip --- static/app/components/gridEditable/index.tsx | 2 +- .../appStarts/components/tables/screensTable.tsx | 4 ++-- .../common/components/tables/screensTable.spec.tsx | 4 ++-- .../mobile/common/components/tables/screensTable.tsx | 12 ++++++------ .../ui/components/tables/spanOperationTable.tsx | 4 ++-- .../mobile/ui/components/tables/uiScreensTable.tsx | 4 ++-- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/static/app/components/gridEditable/index.tsx b/static/app/components/gridEditable/index.tsx index 9cc9203b69c578..8bcc8815767bde 100644 --- a/static/app/components/gridEditable/index.tsx +++ b/static/app/components/gridEditable/index.tsx @@ -45,7 +45,7 @@ export type GridColumn = { export type GridColumnHeader = GridColumn & { name: string; - toolTip?: string; + tooltip?: string; }; export type GridColumnOrder = GridColumnHeader; diff --git a/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx b/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx index bcab565d0c55ec..2fbabde92a8d0a 100644 --- a/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx +++ b/static/app/views/insights/mobile/appStarts/components/tables/screensTable.tsx @@ -64,7 +64,7 @@ export function AppStartScreens({data, eventView, isLoading, pageLinks}: Props) 'count_starts(measurements.app_start_warm)': t('Warm Start Count'), }; - const columnToolTipMap = { + const columnTooltipMap = { [`avg_compare(measurements.app_start_cold,release,${primaryRelease},${secondaryRelease})`]: t('Average Cold Start difference'), [`avg_compare(measurements.app_start_warm,release,${primaryRelease},${secondaryRelease})`]: @@ -128,7 +128,7 @@ export function AppStartScreens({data, eventView, isLoading, pageLinks}: Props) return ( { columnNameMap={{ transaction: 'Screen', }} - columnToolTipMap={{}} + columnTooltipMap={{}} columnOrder={['transaction']} data={{ data: [{id: '1', transaction: 'Screen 1'}], @@ -57,7 +57,7 @@ describe('ScreensTable', () => { columnNameMap={{ transaction: 'Screen', }} - columnToolTipMap={{}} + columnTooltipMap={{}} columnOrder={['transaction', 'non-custom']} data={{ data: [ diff --git a/static/app/views/insights/mobile/common/components/tables/screensTable.tsx b/static/app/views/insights/mobile/common/components/tables/screensTable.tsx index 869f1d9ad61fc9..b165aec32d871e 100644 --- a/static/app/views/insights/mobile/common/components/tables/screensTable.tsx +++ b/static/app/views/insights/mobile/common/components/tables/screensTable.tsx @@ -25,7 +25,7 @@ import type {ModuleName} from 'sentry/views/insights/types'; type Props = { columnNameMap: Record; columnOrder: string[]; - columnToolTipMap: Record | undefined; + columnTooltipMap: Record | undefined; data: TableData | undefined; defaultSort: GridColumnSortBy[]; eventView: EventView; @@ -45,7 +45,7 @@ export function ScreensTable({ pageLinks, columnNameMap, columnOrder, - columnToolTipMap, + columnTooltipMap, defaultSort, customBodyCellRenderer, moduleName, @@ -121,7 +121,7 @@ export function ScreensTable({ /> ); - function columnWithToolTip(tooltipTitle: string) { + function columnWithTooltip(tooltipTitle: string) { return ( {tooltipTitle}}> @@ -131,8 +131,8 @@ export function ScreensTable({ ); } - if (column.toolTip) { - return columnWithToolTip(column.toolTip); + if (column.tooltip) { + return columnWithTooltip(column.tooltip); } return sortLink; } @@ -147,7 +147,7 @@ export function ScreensTable({ key: columnKey, name: columnNameMap[columnKey], width: COL_WIDTH_UNDEFINED, - toolTip: columnToolTipMap ? columnToolTipMap[columnKey] : undefined, + toolTip: columnTooltipMap ? columnTooltipMap[columnKey] : undefined, }; })} columnSortBy={defaultSort} diff --git a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx index 608b4642477848..b5653d9f33cd33 100644 --- a/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx +++ b/static/app/views/insights/mobile/ui/components/tables/spanOperationTable.tsx @@ -119,7 +119,7 @@ export function SpanOperationTable({ t('Change'), }; - const columnToolTipMap = { + const columnTooltipMap = { [`division_if(mobile.slow_frames,mobile.total_frames,release,${primaryRelease})`]: t( 'The number of slow frames divided by total frames (%s)', PRIMARY_RELEASE_ALIAS @@ -179,7 +179,7 @@ export function SpanOperationTable({ return ( Date: Tue, 23 Jul 2024 17:30:47 +0200 Subject: [PATCH 5/5] Address PR feedback --- static/app/components/gridEditable/index.tsx | 1 - static/app/utils/discover/fieldRenderers.tsx | 4 --- .../components/tables/screensTable.spec.tsx | 33 ++++++++++++++++++- .../common/components/tables/screensTable.tsx | 6 ++-- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/static/app/components/gridEditable/index.tsx b/static/app/components/gridEditable/index.tsx index 8bcc8815767bde..e301366dd19a62 100644 --- a/static/app/components/gridEditable/index.tsx +++ b/static/app/components/gridEditable/index.tsx @@ -45,7 +45,6 @@ export type GridColumn = { export type GridColumnHeader = GridColumn & { name: string; - tooltip?: string; }; export type GridColumnOrder = GridColumnHeader; diff --git a/static/app/utils/discover/fieldRenderers.tsx b/static/app/utils/discover/fieldRenderers.tsx index d75c91dc9717af..7ab9d686f0d33e 100644 --- a/static/app/utils/discover/fieldRenderers.tsx +++ b/static/app/utils/discover/fieldRenderers.tsx @@ -819,10 +819,6 @@ export function getSortField( return field; } - if (field.startsWith('division_if(')) { - return field; - } - for (const alias in AGGREGATIONS) { if (field.startsWith(alias)) { return AGGREGATIONS[alias].isSortable ? field : null; diff --git a/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx b/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx index 35d247adf015b0..5acf95be9e863b 100644 --- a/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx +++ b/static/app/views/insights/mobile/common/components/tables/screensTable.spec.tsx @@ -1,4 +1,4 @@ -import {render, screen} from 'sentry-test/reactTestingLibrary'; +import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; import EventView from 'sentry/utils/discover/eventView'; import {MutableSearch} from 'sentry/utils/tokenizeSearch'; @@ -84,4 +84,35 @@ describe('ScreensTable', () => { expect(screen.getByText('Custom rendered Screen 1')).toBeInTheDocument(); expect(screen.getByText('non customized value')).toBeInTheDocument(); }); + + it('renders column header tooltips', async () => { + render( + + ); + + const columnHeader = screen.getByText('Screen Column'); + await userEvent.hover(columnHeader); + + expect(await screen.findByText('Screen Column Tooltip')).toBeInTheDocument(); + }); }); diff --git a/static/app/views/insights/mobile/common/components/tables/screensTable.tsx b/static/app/views/insights/mobile/common/components/tables/screensTable.tsx index b165aec32d871e..bba1999085b9e0 100644 --- a/static/app/views/insights/mobile/common/components/tables/screensTable.tsx +++ b/static/app/views/insights/mobile/common/components/tables/screensTable.tsx @@ -131,8 +131,9 @@ export function ScreensTable({ ); } - if (column.tooltip) { - return columnWithTooltip(column.tooltip); + const tooltip = columnTooltipMap ? columnTooltipMap[column.key] : undefined; + if (tooltip) { + return columnWithTooltip(tooltip); } return sortLink; } @@ -147,7 +148,6 @@ export function ScreensTable({ key: columnKey, name: columnNameMap[columnKey], width: COL_WIDTH_UNDEFINED, - toolTip: columnTooltipMap ? columnTooltipMap[columnKey] : undefined, }; })} columnSortBy={defaultSort}