-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ref(insights): Refactor Resource Summary + Landing charts into separate modules #89612
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.
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
Changes from 3 commits
5da6791
ecd50f8
bd878a0
792b4a5
6582cea
b36d178
4090c5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ import {trackAnalytics} from 'sentry/utils/analytics'; | |
import {useLocation} from 'sentry/utils/useLocation'; | ||
import {useNavigate} from 'sentry/utils/useNavigate'; | ||
import useOrganization from 'sentry/utils/useOrganization'; | ||
import {getResourceTypeFilter} from 'sentry/views/insights/browser/common/queries/useResourcesQuery'; | ||
import RenderBlockingSelector from 'sentry/views/insights/browser/resources/components/renderBlockingSelector'; | ||
import ResourceTable from 'sentry/views/insights/browser/resources/components/tables/resourceTable'; | ||
import { | ||
|
@@ -24,16 +23,13 @@ import { | |
import {useResourceSort} from 'sentry/views/insights/browser/resources/utils/useResourceSort'; | ||
import {QueryParameterNames} from 'sentry/views/insights/common/views/queryParameters'; | ||
import {TransactionSelector} from 'sentry/views/insights/common/views/spans/selectors/transactionSelector'; | ||
import type {ModuleFilters} from 'sentry/views/insights/common/views/spans/types'; | ||
|
||
import {ResourceLandingPageCharts} from './charts/resourceLandingPageCharts'; | ||
|
||
const { | ||
SPAN_OP: RESOURCE_TYPE, | ||
SPAN_DOMAIN, | ||
TRANSACTION, | ||
RESOURCE_RENDER_BLOCKING_STATUS, | ||
USER_GEO_SUBREGION, | ||
} = BrowserStarfishFields; | ||
|
||
type Option = { | ||
|
@@ -45,25 +41,10 @@ function ResourceView() { | |
const filters = useResourceModuleFilters(); | ||
const sort = useResourceSort(); | ||
|
||
const spanTimeChartsFilters: ModuleFilters = { | ||
'span.op': `[${DEFAULT_RESOURCE_TYPES.join(',')}]`, | ||
...(filters[SPAN_DOMAIN] ? {[SPAN_DOMAIN]: filters[SPAN_DOMAIN]} : {}), | ||
}; | ||
|
||
const extraQuery = [ | ||
...getResourceTypeFilter(undefined, DEFAULT_RESOURCE_TYPES), | ||
...(filters[USER_GEO_SUBREGION] | ||
? [`user.geo.subregion:[${filters[USER_GEO_SUBREGION].join(',')}]`] | ||
: []), | ||
]; | ||
|
||
return ( | ||
<Fragment> | ||
<SpanTimeChartsContainer> | ||
<ResourceLandingPageCharts | ||
appliedFilters={spanTimeChartsFilters} | ||
extraQuery={extraQuery} | ||
Comment on lines
-64
to
-65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved these into the new widget |
||
/> | ||
<ResourceLandingPageCharts /> | ||
</SpanTimeChartsContainer> | ||
|
||
<DropdownContainer> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,7 +155,7 @@ function ResourceSummary() { | |
</ModuleLayout.Full> | ||
)} | ||
|
||
<ResourceSummaryCharts groupId={groupId!} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved into widget, |
||
<ResourceSummaryCharts /> | ||
|
||
<ModuleLayout.Full> | ||
<ResourceSummaryTable /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import type {PageFilters} from 'sentry/types/core'; | ||
import {EMPTY_OPTION_VALUE, MutableSearch} from 'sentry/utils/tokenizeSearch'; | ||
import {getResourceTypeFilter} from 'sentry/views/insights/browser/common/queries/useResourcesQuery'; | ||
import {DEFAULT_RESOURCE_TYPES} from 'sentry/views/insights/browser/resources/settings'; | ||
import { | ||
BrowserStarfishFields, | ||
useResourceModuleFilters, | ||
} from 'sentry/views/insights/browser/resources/utils/useResourceFilters'; | ||
import {useSpanMetricsSeries} from 'sentry/views/insights/common/queries/useDiscoverSeries'; | ||
import type {ModuleFilters} from 'sentry/views/insights/common/views/spans/types'; | ||
import {SpanMetricsField} from 'sentry/views/insights/types'; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. had to alias There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DominikB2014 Oops, should have looked into the enums... updated! |
||
|
||
const SPAN_FILTER_KEYS = ['span_operation', SPAN_DOMAIN, 'action']; | ||
|
||
const buildDiscoverQueryConditions = (appliedFilters: ModuleFilters) => { | ||
const result = Object.keys(appliedFilters) | ||
.filter(key => SPAN_FILTER_KEYS.includes(key)) | ||
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message | ||
.filter(key => Boolean(appliedFilters[key])) | ||
.map(key => { | ||
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message | ||
const value = appliedFilters[key]; | ||
if (key === SPAN_DOMAIN && value === EMPTY_OPTION_VALUE) { | ||
return [`!has:${SPAN_DOMAIN}`]; | ||
} | ||
return `${key}:${value}`; | ||
}); | ||
|
||
result.push(`has:${NORMALIZED_DESCRIPTION}`); | ||
|
||
return result.join(' '); | ||
billyvg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
interface Props { | ||
pageFilters?: PageFilters; | ||
} | ||
|
||
export function useResourceLandingSeries(props?: Props) { | ||
const filters = useResourceModuleFilters(); | ||
|
||
const spanTimeChartsFilters: ModuleFilters = { | ||
'span.op': `[${DEFAULT_RESOURCE_TYPES.join(',')}]`, | ||
...(filters[STARFISH_SPAN_DOMAIN] | ||
? {[STARFISH_SPAN_DOMAIN]: filters[STARFISH_SPAN_DOMAIN]} | ||
: {}), | ||
}; | ||
|
||
const extraQuery = [ | ||
...getResourceTypeFilter(undefined, DEFAULT_RESOURCE_TYPES), | ||
...(filters[USER_GEO_SUBREGION] | ||
? [`user.geo.subregion:[${filters[USER_GEO_SUBREGION].join(',')}]`] | ||
billyvg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: []), | ||
]; | ||
|
||
let query: string = buildDiscoverQueryConditions(spanTimeChartsFilters); | ||
|
||
if (extraQuery) { | ||
query += ` ${extraQuery.join(' ')}`; | ||
} | ||
billyvg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return useSpanMetricsSeries( | ||
{ | ||
search: new MutableSearch(query), | ||
yAxis: ['epm()', `avg(${SPAN_SELF_TIME})`], | ||
transformAliasToInputFormat: true, | ||
}, | ||
'api.starfish.span-time-charts', | ||
props.pageFilters | ||
Check failure on line 71 in static/app/views/insights/common/components/widgets/hooks/useResourceLandingSeries.tsx
|
||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import type {PageFilters} from 'sentry/types/core'; | ||
import {MutableSearch} from 'sentry/utils/tokenizeSearch'; | ||
import {Referrer} from 'sentry/views/insights/browser/resources/referrer'; | ||
import {useResourceModuleFilters} from 'sentry/views/insights/browser/resources/utils/useResourceFilters'; | ||
import {useSpanMetricsSeries} from 'sentry/views/insights/common/queries/useDiscoverSeries'; | ||
import {SpanMetricsField} from 'sentry/views/insights/types'; | ||
|
||
const { | ||
SPAN_SELF_TIME, | ||
HTTP_RESPONSE_CONTENT_LENGTH, | ||
HTTP_DECODED_RESPONSE_CONTENT_LENGTH, | ||
HTTP_RESPONSE_TRANSFER_SIZE, | ||
RESOURCE_RENDER_BLOCKING_STATUS, | ||
} = SpanMetricsField; | ||
|
||
interface Props { | ||
groupId?: string; | ||
pageFilters?: PageFilters; | ||
} | ||
|
||
export function useResourceSummarySeries({pageFilters, groupId}: Props) { | ||
const filters = useResourceModuleFilters(); | ||
|
||
const mutableSearch = MutableSearch.fromQueryObject({ | ||
'span.group': groupId, | ||
...(filters[RESOURCE_RENDER_BLOCKING_STATUS] | ||
? { | ||
[RESOURCE_RENDER_BLOCKING_STATUS]: filters[RESOURCE_RENDER_BLOCKING_STATUS], | ||
} | ||
: {}), | ||
...(filters[SpanMetricsField.USER_GEO_SUBREGION] | ||
? { | ||
[SpanMetricsField.USER_GEO_SUBREGION]: `[${filters[SpanMetricsField.USER_GEO_SUBREGION].join(',')}]`, | ||
} | ||
: {}), | ||
}); | ||
|
||
return useSpanMetricsSeries( | ||
{ | ||
search: mutableSearch, | ||
yAxis: [ | ||
`epm()`, | ||
`avg(${SPAN_SELF_TIME})`, | ||
`avg(${HTTP_RESPONSE_CONTENT_LENGTH})`, | ||
`avg(${HTTP_DECODED_RESPONSE_CONTENT_LENGTH})`, | ||
`avg(${HTTP_RESPONSE_TRANSFER_SIZE})`, | ||
billyvg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
], | ||
enabled: Boolean(groupId), | ||
transformAliasToInputFormat: true, | ||
}, | ||
Referrer.RESOURCE_SUMMARY_CHARTS, | ||
pageFilters | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).