ref(insights): Refactor Resource Summary + Landing charts into separate modules #89612
Conversation
github-actions
bot
added
the
Scope: Frontend
billyvg
changed the title
ResourceSummaryCharts into separate modules
billyvg
force-pushed
the
ref-insights-combine-widgets-browser-resources
branch
from
3255226 to
5da6791
Compare
|
@codecov-ai-reviewer review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
![codecov-ai[bot]](https://avatars.githubusercontent.com/in/797565?s=60&v=4){kind=small}
static/app/views/insights/common/components/widgets/hooks/useResourceLandingSeries.tsx
Show resolved
Hide resolved
static/app/views/insights/common/components/widgets/hooks/useResourceLandingSeries.tsx
Show resolved
Hide resolved
| if (data) { | ||
| data[`avg(${HTTP_RESPONSE_TRANSFER_SIZE})`].lineStyle = { | ||
| type: 'dashed', | ||
| }; | ||
| data[`avg(${HTTP_DECODED_RESPONSE_CONTENT_LENGTH})`].lineStyle = { | ||
| type: 'dashed', | ||
| }; |
There was a problem hiding this comment.
Line style modifications directly mutate the data object. It's better to create a new transformed series object to maintain data immutability.
static/app/views/insights/common/components/widgets/hooks/useResourceSummarySeries.tsx
Show resolved
Hide resolved
| {...props} | ||
| id="resourceLandingDurationChartWidget" | ||
| title={getDurationChartTitle('resource')} | ||
| series={[data[`avg(${SPAN_SELF_TIME})`]]} | ||
| isLoading={isPending} | ||
| error={error} |
There was a problem hiding this comment.
Add a useCallback for the series array construction to prevent unnecessary re-renders in the InsightsLineChartWidget component.
| appliedFilters={spanTimeChartsFilters} | ||
| extraQuery={extraQuery} |
There was a problem hiding this comment.
Moved these into the new widget
| @@ -155,7 +155,7 @@ function ResourceSummary() { | |||
| </ModuleLayout.Full> | |||
| )} | |||
|
|
|||
| <ResourceSummaryCharts groupId={groupId!} /> | |||
There was a problem hiding this comment.
Moved into widget, groupId comes from URL params - currently drawer is directly tied to the chart/page where it was opened, so i think it's fine to have the widget grab this from the URL/router. if there was some world where we would render these charts separate independently, then we'd need to refactor to allow a way to specify the required properties that we'd need to add to the URL for linking and for rendering.
| const {SPAN_SELF_TIME, NORMALIZED_DESCRIPTION, SPAN_DOMAIN} = SpanMetricsField; | ||
| const {SPAN_DOMAIN: STARFISH_SPAN_DOMAIN, USER_GEO_SUBREGION} = BrowserStarfishFields; |
There was a problem hiding this comment.
had to alias BrowserStarfishFields.SPAN_DOMAIN since we moved these down into the new hook and now there are 2 SPAN_DOMAINs in scope.
There was a problem hiding this comment.
BrowserStarfishFields is in the process of being deprecated, could we just grab all of these fields from SpanMetricsFeild ?
There was a problem hiding this comment.
@DominikB2014 Oops, should have looked into the enums... updated!
static/app/views/insights/common/components/widgets/hooks/useResourceLandingSeries.tsx
Show resolved
Hide resolved
| if (spanMetricsSeriesData) { | ||
| spanMetricsSeriesData[`avg(${HTTP_RESPONSE_TRANSFER_SIZE})`].lineStyle = { | ||
| type: 'dashed', | ||
| }; | ||
| spanMetricsSeriesData[`avg(${HTTP_DECODED_RESPONSE_CONTENT_LENGTH})`].lineStyle = { | ||
| type: 'dashed', | ||
| }; | ||
| } |
There was a problem hiding this comment.
All 3 charts shared the data fetching code, but in the new charts only the AverageSeries chart has this code since it is the only chart that uses these series (IIUC).
| const {SPAN_SELF_TIME, NORMALIZED_DESCRIPTION, SPAN_DOMAIN} = SpanMetricsField; | ||
| const {SPAN_DOMAIN: STARFISH_SPAN_DOMAIN, USER_GEO_SUBREGION} = BrowserStarfishFields; |
There was a problem hiding this comment.
BrowserStarfishFields is in the process of being deprecated, could we just grab all of these fields from SpanMetricsFeild ?
static/app/views/insights/common/components/widgets/hooks/useResourceLandingSeries.tsx
Outdated
Show resolved
Hide resolved
…esourceLandingSeries.tsx
Part of #88560