Skip to content

fix(perf): Perf Landing v3 UI Tweaks #29812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k-fish merged 6 commits into from
Nov 5, 2021
Merged

fix(perf): Perf Landing v3 UI Tweaks #29812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf06571
fix(perf): Disable zoom in histogram widget
k-fish Nov 5, 2021
f9081cb
Add additional widget types to pageload and frontend other views
k-fish Nov 5, 2021
db83e0c
Fix reloading when switching periods for singleFieldAreaWidget
k-fish Nov 5, 2021
58dd9c3
Stop issues breaking when temporarily switching to it by catching if …
k-fish Nov 5, 2021
f5e5dcf
Only pick required props for EventsRequest
k-fish Nov 5, 2021
02fc67a
Have condition to remove 0 value items from the list widgets also app…
k-fish Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion static/app/views/performance/landing/chart/histogramChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ type ChartProps = {
height?: number;
grid?: BarChart['props']['grid'];
disableXAxis?: boolean;
disableZoom?: boolean;
colors?: string[];
};

Expand All @@ -132,6 +133,7 @@ export function Chart(props: ChartProps) {
height,
grid,
disableXAxis,
disableZoom,
colors,
} = props;
if (!chartData) {
Expand Down Expand Up @@ -199,7 +201,7 @@ export function Chart(props: ChartProps) {
}
}
stacked
{...zoomRenderProps}
{...(disableZoom ? {} : zoomRenderProps)}
/>
),
fixed: <Placeholder height="250px" testId="skeleton-ui" />,
Expand Down
2 changes: 2 additions & 0 deletions static/app/views/performance/landing/views/frontendOtherView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ export function FrontendOtherView(props: BasePerformanceViewProps) {
allowedCharts={[
PerformanceWidgetSetting.MOST_RELATED_ERRORS,
PerformanceWidgetSetting.MOST_RELATED_ISSUES,
PerformanceWidgetSetting.SLOW_HTTP_OPS,
PerformanceWidgetSetting.SLOW_RESOURCE_OPS,
]}
/>
<Table
Expand Down
3 changes: 3 additions & 0 deletions static/app/views/performance/landing/views/frontendPageloadView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ export function FrontendPageloadView(props: BasePerformanceViewProps) {
PerformanceWidgetSetting.MOST_RELATED_ERRORS,
PerformanceWidgetSetting.MOST_RELATED_ISSUES,
PerformanceWidgetSetting.WORST_LCP_VITALS,
PerformanceWidgetSetting.SLOW_HTTP_OPS,
PerformanceWidgetSetting.SLOW_BROWSER_OPS,
PerformanceWidgetSetting.SLOW_RESOURCE_OPS,
]}
/>
<Table
Expand Down
3 changes: 2 additions & 1 deletion static/app/views/performance/landing/widgets/widgets/histogramWidget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export function HistogramWidget(props: Props) {
transform: transformHistogramQuery,
},
};
}, [props.eventView.query, props.fields[0], props.organization.slug]);
}, [props.eventView, props.fields[0], props.organization.slug]);

const onFilterChange = () => {};

Expand Down Expand Up @@ -80,6 +80,7 @@ export function HistogramWidget(props: Props) {
field={props.fields[0]}
chartData={provided.widgetData.chart?.data?.[props.fields[0]]}
disableXAxis
disableZoom
Copy link
Member

@Zylphrex Zylphrex Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've never had an un-zoom feature, is there a reason why it's a necessity now?

Copy link
Member Author

@k-fish k-fish Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can zoom in a bunch of the widgets but there is no way to reset and unzoom, we're also currently not making 2nd requests to get the zoomed in data, I'd rather just exclude the ability for now.

/>
),
height: 160,
Expand Down
16 changes: 12 additions & 4 deletions static/app/views/performance/landing/widgets/widgets/lineChartListWidget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {Fragment, FunctionComponent, useMemo, useState} from 'react';
import {withRouter} from 'react-router';
import styled from '@emotion/styled';
import {Location} from 'history';
import pick from 'lodash/pick';

import _EventsRequest from 'app/components/charts/eventsRequest';
import {getInterval} from 'app/components/charts/utils';
Expand Down Expand Up @@ -92,7 +93,6 @@ export function LineChartListWidget(props: Props) {
eventView.query = mutableSearch.formatString();
} else if (isSlowestType) {
eventView.additionalConditions.setFilterValues('epm()', ['>0.01']);
eventView.additionalConditions.setFilterValues(field, ['>0']);
eventView.fields = [
{field: 'transaction'},
{field: 'project.id'},
Expand All @@ -103,6 +103,8 @@ export function LineChartListWidget(props: Props) {
// Most related errors
eventView.fields = [{field: 'transaction'}, {field: 'project.id'}, {field}];
}
// Don't retrieve list items with 0 in the field.
eventView.additionalConditions.setFilterValues(field, ['>0']);
return (
<DiscoverQuery
{...provided}
Expand All @@ -114,7 +116,7 @@ export function LineChartListWidget(props: Props) {
},
transform: transformDiscoverToList,
}),
[props.eventView.query, field, props.organization.slug]
[props.eventView, field, props.organization.slug]
);

const chartQuery = useMemo<QueryDefinition<DataType, WidgetDataResult>>(() => {
Expand All @@ -125,10 +127,16 @@ export function LineChartListWidget(props: Props) {
fields: field,
component: provided => {
const eventView = props.eventView.clone();
if (!provided.widgetData.list.data[selectedListIndex]?.transaction) {
return null;
}
eventView.additionalConditions.setFilterValues('transaction', [
provided.widgetData.list.data[selectedListIndex].transaction as string,
]);
if (props.chartSetting === PerformanceWidgetSetting.MOST_RELATED_ISSUES) {
if (!provided.widgetData.list.data[selectedListIndex]?.issue) {
return null;
}
eventView.fields = [
{field: 'issue'},
{field: 'issue.id'},
Expand All @@ -148,7 +156,7 @@ export function LineChartListWidget(props: Props) {
}
return (
<EventsRequest
{...provided}
{...pick(provided, ['children', 'organization', 'yAxis'])}
limit={1}
includePrevious
includeTransformedData
Expand All @@ -168,7 +176,7 @@ export function LineChartListWidget(props: Props) {
},
transform: transformEventsRequestToArea,
};
}, [props.eventView.query, field, props.organization.slug, selectedListIndex]);
}, [props.eventView, field, props.organization.slug, selectedListIndex]);
Copy link
Member

@Zylphrex Zylphrex Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say with confidence that I have a strong grasp on what this will do but won't this trigger a re-render on every location change because eventView is re-derived each time, resulting in a new reference?

Copy link
Member Author

@k-fish k-fish Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it schedules a rerender, but eventView is no longer being passed to the query component, so the component itself never refires. Re-renders are fine, but re-firing the api request is not. The eventView has more than just the query to look for (in this case statsPeriod was changing), I'd rather not figure out primitives for every facet of eventView that could change and just let the existing component refetch mechanism deal with it.


const Queries = {
list: listQuery,
Expand Down
66 changes: 35 additions & 31 deletions static/app/views/performance/landing/widgets/widgets/singleFieldAreaWidget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {Fragment, FunctionComponent, useMemo} from 'react';
import {withRouter} from 'react-router';
import styled from '@emotion/styled';
import {Location} from 'history';
import pick from 'lodash/pick';

import _EventsRequest from 'app/components/charts/eventsRequest';
import {getInterval} from 'app/components/charts/utils';
Expand All @@ -13,7 +14,7 @@ import _DurationChart from 'app/views/performance/charts/chart';

import {GenericPerformanceWidget} from '../components/performanceWidget';
import {transformEventsRequestToArea} from '../transforms/transformEventsToArea';
import {WidgetDataResult} from '../types';
import {QueryDefinition, WidgetDataResult} from '../types';

type Props = {
title: string;
Expand All @@ -28,7 +29,7 @@ type Props = {
ContainerActions: FunctionComponent<{isLoading: boolean}>;
};

type AreaDataType = {
type DataType = {
chart: WidgetDataResult & ReturnType<typeof transformEventsRequestToArea>;
};

Expand All @@ -39,38 +40,41 @@ export function SingleFieldAreaWidget(props: Props) {
if (props.fields.length !== 1) {
throw new Error(`Single field area can only accept a single field (${props.fields})`);
}
const field = props.fields[0];

const Queries = useMemo(() => {
return {
chart: {
fields: props.fields[0],
component: provided => (
<EventsRequest
{...provided}
limit={1}
includePrevious
includeTransformedData
partial
currentSeriesName={props.fields[0]}
eventView={props.eventView}
query={props.eventView.getQueryWithAdditionalConditions()}
interval={getInterval(
{
start: provided.start,
end: provided.end,
period: provided.period,
},
'medium'
)}
/>
),
transform: transformEventsRequestToArea,
},
};
}, [props.eventView.query, props.fields[0], props.organization.slug]);
const chart = useMemo<QueryDefinition<DataType, WidgetDataResult>>(
Copy link
Member

@Zylphrex Zylphrex Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why the useMemo here is needed. If you wrap the memorized result inside of an object like you have below, it will always trigger a re-render of the child component since the reference to Queries will be different even if chart has been memorized right?

Copy link
Member Author

@k-fish k-fish Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-rendering the same component is fine now, building a new component inside an object each render isn't. Since it wasn't part of the jsx in the return iirc it was being rebuilt each time.

() => ({
fields: props.fields[0],
component: provided => (
<EventsRequest
{...pick(provided, ['children', 'organization', 'yAxis'])}
limit={1}
includePrevious
includeTransformedData
partial
currentSeriesNames={[field]}
query={props.eventView.getQueryWithAdditionalConditions()}
interval={getInterval(
{
start: provided.start,
end: provided.end,
period: provided.period,
},
'medium'
)}
/>
),
transform: transformEventsRequestToArea,
}),
[props.eventView, field, props.organization.slug]
);

const Queries = {
chart,
};

return (
<GenericPerformanceWidget<AreaDataType>
<GenericPerformanceWidget<DataType>
{...props}
Subtitle={() => (
<Subtitle>{t('Compared to last %s ', globalSelection.datetime.period)}</Subtitle>
Expand Down
2 changes: 1 addition & 1 deletion static/app/views/performance/landing/widgets/widgets/trendsWidget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export function TrendsWidget(props: Props) {
),
transform: transformTrendsDiscover,
}),
[eventView.query, eventView.fields, trendChangeType]
[eventView, trendChangeType]
);

const Queries = {
Expand Down
10 changes: 2 additions & 8 deletions tests/js/spec/views/performance/landing/widgets/widgetContainer.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,8 @@ describe('Performance > Widgets > WidgetContainer', function () {
expect.anything(),
expect.objectContaining({
query: expect.objectContaining({
environment: [],
interval: '1h',
partial: '1',
project: [],
query: '',
statsPeriod: '28d',
yAxis: 'tpm()',
Expand Down Expand Up @@ -104,10 +102,8 @@ describe('Performance > Widgets > WidgetContainer', function () {
expect.anything(),
expect.objectContaining({
query: expect.objectContaining({
environment: [],
interval: '1h',
partial: '1',
project: [],
query: '',
statsPeriod: '28d',
yAxis: 'failure_rate()',
Expand Down Expand Up @@ -138,10 +134,8 @@ describe('Performance > Widgets > WidgetContainer', function () {
expect.anything(),
expect.objectContaining({
query: expect.objectContaining({
environment: [],
interval: '1h',
partial: '1',
project: [],
query: '',
statsPeriod: '28d',
yAxis: 'user_misery()',
Expand Down Expand Up @@ -260,7 +254,7 @@ describe('Performance > Widgets > WidgetContainer', function () {
field: ['transaction', 'project.id', 'failure_count()'],
per_page: 3,
project: [],
query: '',
query: 'failure_count():>0',
sort: '-failure_count()',
statsPeriod: '14d',
}),
Expand Down Expand Up @@ -294,7 +288,7 @@ describe('Performance > Widgets > WidgetContainer', function () {
field: ['issue', 'transaction', 'title', 'project.id', 'count()'],
per_page: 3,
project: [],
query: 'event.type:error !tags[transaction]:""',
query: 'event.type:error !tags[transaction]:"" count():>0',
sort: '-count()',
statsPeriod: '14d',
}),
Expand Down