feat(perf): Finish remaining landing v3 views #29715
Conversation
This adds a line chart type widget and support for different widget types on each of the views, as well as including multiple small fixes for behaviours noted as bugs. There will be another follow up PR for remaining small issues.
…nges (eg. sorting the table.)
| if (isLineChart) { | ||
| return <LineChart height={height} series={[]} {...areaChartProps} />; | ||
| } |
There was a problem hiding this comment.
Can be a follow-up, but I think we typically use the TransparentLoadingMask over a chart to indicate loading states.
There was a problem hiding this comment.
The transparent loading mask should only appear on reload state, which I'm thinking of handling in performance widget under the hood, instead of in these charts.
| @@ -361,6 +362,7 @@ export const PERFORMANCE_TERMS: Record<PERFORMANCE_TERM, TermFormatter> = { | |||
| frozenFrames: () => t('The count of the number of frozen frames in the transaction.'), | |||
| mostErrors: () => t('Transactions with the most associated errors.'), | |||
| mostIssues: () => t('The most instances of an issue for a related transaction.'), | |||
| slowHTTPSpans: () => t('The transactions with the slowest spans of a certain type.'), | |||
There was a problem hiding this comment.
Should there be custom messages for each type?
There was a problem hiding this comment.
Yep. Thanks for reminding me.
| const slowList = [ | ||
| PerformanceWidgetSetting.SLOW_HTTP_OPS, | ||
| PerformanceWidgetSetting.SLOW_DB_OPS, | ||
| PerformanceWidgetSetting.SLOW_BROWSER_OPS, | ||
| PerformanceWidgetSetting.SLOW_RESOURCE_OPS, | ||
| PerformanceWidgetSetting.MOST_SLOW_FRAMES, | ||
| PerformanceWidgetSetting.MOST_FROZEN_FRAMES, | ||
| ]; | ||
| const isSlowestType = slowList.includes(props.chartSetting); |
There was a problem hiding this comment.
Nit: Can we declare slowList as a set outside of this function
There was a problem hiding this comment.
Yeah, I'm also considering splitting the file. I'm trying to think if I can simplify this memoization code or list visualizations to reduce the boilerplate between widgets first.
| }; | ||
| }, [ | ||
| props.eventView.query, | ||
| props.fields[0], |
There was a problem hiding this comment.
Nit: you use props.fields[0] a lot in this function and I see that you have an assertion for props.fields being a list of a single item. Can we do something like
const field = props.fields[0];After the assertion and use that throughout the function? It would clear up any concerns that there props.fields is a list of multiple items.
There was a problem hiding this comment.
Yep, great call. Might switch this when I normalize how widget specializations get called so it's always provided as the first field from the widget definition for all specializations.
static/app/views/performance/landing/widgets/widgets/lineChartListWidget.tsx
Show resolved
Hide resolved
| @@ -86,7 +86,7 @@ export function TrendsWidget(props: Props) { | |||
| transform: transformTrendsDiscover, | |||
| }, | |||
| }; | |||
| }, [_eventView, trendChangeType]); | |||
| }, [eventView.query, eventView.fields, trendChangeType]); | |||
There was a problem hiding this comment.
I may be wrong but since eventView is derived from the location each time, wouldn't this cause useMemo to re-fire on every location change regardless if the fields changed or not?
There was a problem hiding this comment.
I still have to write some tests to ensure memoization is working correctly, likely as part of the batch query handler PR, but I think this should be fine for now. eventView.query is a string, which can be compared as a primitive, which means it shouldn't cause it to refire if eventView is recreated, which is what was happening before (sorting the table caused widgets to reload... not ideal)
….tsx Co-authored-by: Tony Xiao <txiao@sentry.io>
…ry/sentry into feat/perf-landing-finish-views-2
Summary
This adds a line chart type widget and support for different widget types on each of the views, as well as including multiple small fixes for behaviours noted as bugs. There will be another follow up PR for remaining small issues.
Other