ref(insights): Combine data loading + widget rendering for Insights -> Http pages #89325

billyvg · 2025-04-10T19:23:56Z

As part of an effort to be able to deeplink to charts via the Releases Drawer, we need to move data fetching + chart rendering into a self-contained component (with a unique id).

This refactors the widgets on httpLandingPage and httpDomainSummaryPage so that we get an idea of what this will look like before continuing on to do others.

…> Http pages As part of an effort to be able to [deeplink to charts via the Releases Drawer](#88560), we need to move data fetching + chart rendering into a self-contained component (with a unique id). This refactors the widgets on `httpLandingPage` and `httpDomainSummaryPage` so that we get an idea of what this will look like.

billyvg · 2025-04-10T19:27:03Z

static/app/views/insights/common/components/widgets/hooks/useHttpDomainSummaryFilter.tsx

+import {BASE_FILTERS} from 'sentry/views/insights/http/settings';
+import {SpanMetricsField} from 'sentry/views/insights/types';
+
+export function useHttpDomainSummaryFilter() {


Are these generally pretty static per "page"?

Generally, all charts at the top of a page share filters, and the table under them has its own set of filters that's close to, but not exactly the same as the chart filters. So, this hook might be better if the word Chart appeared in it?

static/app/views/insights/common/components/widgets/httpDomainSummaryDurationWidget.tsx

billyvg · 2025-04-10T19:29:23Z

static/app/views/insights/common/components/widgets/httpDomainSummaryDurationWidget.tsx

+    isPending: isDurationDataLoading,
+    data: durationData,
+    error: durationError,


probably don't need these aliases? maybe makes it easier to read the diffs as we're transitioning

You can remove the aliases if you want. The aliases help a lot when fetching multiple series per page

Gonna leave them for now, we can go back to update it later

billyvg · 2025-04-10T19:31:08Z

static/app/views/insights/http/views/httpDomainSummaryPage.tsx

@@ -22,14 +18,15 @@ import {ModulePageProviders} from 'sentry/views/insights/common/components/modul
 import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/moduleUpsellHookWrapper';
 import {ReadoutRibbon, ToolRibbon} from 'sentry/views/insights/common/components/ribbon';
 import {getTimeSpentExplanation} from 'sentry/views/insights/common/components/tableCells/timeSpentCell';
+import {useHttpDomainSummaryFilter} from 'sentry/views/insights/common/components/widgets/hooks/useHttpDomainSummaryFilter';
+import HttpDomainSummaryDurationWidget from 'sentry/views/insights/common/components/widgets/httpDomainSummaryDurationWidget';


See https://github.com/getsentry/sentry/pull/89045/files#diff-886e46c8485496b33137c9770f4eb4fb6e1085c7e6e0fbafdd5721d4310b74e3R33-R62 on how we're going to use these widgets -- these are default exports to make it easier to dynamically import and render.

billyvg · 2025-04-10T19:32:59Z

static/app/views/insights/http/views/httpLandingPage.spec.tsx

@@ -260,6 +260,8 @@ describe('HTTPLandingPage', function () {
  it('fetches module data', async function () {
    render(<HTTPLandingPage />, {organization});

+    await waitForElementToBeRemoved(() => screen.queryAllByTestId('loading-indicator'));


Oh this was from the PoC where we dynamically loaded the widgets. Still makes sense here that we'd wait for loading components to go away before we check the mocked requests.

gggritso

Great! I'm happy to mash approve, but I also wanted to ask about the names. Tables are also technically widgets, so I think it'd be clearer if the components and their IDs has Chart in them somewhere, or were namespaced in a folder that specified they're charts. Not huge, but would help!

gggritso · 2025-04-10T20:50:12Z

static/app/views/insights/common/components/widgets/hooks/useHttpDomainSummaryFilter.tsx

+import {BASE_FILTERS} from 'sentry/views/insights/http/settings';
+import {SpanMetricsField} from 'sentry/views/insights/types';
+
+export function useHttpDomainSummaryFilter() {


Generally, all charts at the top of a page share filters, and the table under them has its own set of filters that's close to, but not exactly the same as the chart filters. So, this hook might be better if the word Chart appeared in it?

gggritso · 2025-04-10T20:52:38Z

static/app/views/insights/common/components/widgets/httpDomainSummaryDurationWidget.tsx

+    isPending: isDurationDataLoading,
+    data: durationData,
+    error: durationError,


You can remove the aliases if you want. The aliases help a lot when fetching multiple series per page

static/app/views/insights/common/components/widgets/httpDomainSummaryDurationWidget.tsx

billyvg · 2025-04-11T14:19:15Z

static/app/views/insights/http/views/httpDomainSummaryPage.tsx

lol Cursor decided to rewrite this file?????????????????????

…art-rendering

billyvg · 2025-04-11T15:04:25Z

Renamed everything to include Chart in its names

This broke in #89325 We should be passing in `location.query.domainsSort` into decodeSorts and not the string `domainsSort`! Also added a test case to ensure sort always works off the query param

Part of #88560 Follow-up to #89325 --------- Co-authored-by: George Gritsouk <989898+gggritso@users.noreply.github.com>

…> Http pages (#89325) As part of an effort to be able to [deeplink to charts via the Releases Drawer](#88560), we need to move data fetching + chart rendering into a self-contained component (with a unique id). This refactors the widgets on `httpLandingPage` and `httpDomainSummaryPage` so that we get an idea of what this will look like before continuing on to do others.

This broke in #89325 We should be passing in `location.query.domainsSort` into decodeSorts and not the string `domainsSort`! Also added a test case to ensure sort always works off the query param

Part of #88560 Follow-up to #89325 --------- Co-authored-by: George Gritsouk <989898+gggritso@users.noreply.github.com>

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 10, 2025

vercel bot deployed to Preview April 10, 2025 19:28 View deployment

billyvg commented Apr 10, 2025

View reviewed changes

billyvg marked this pull request as ready for review April 10, 2025 19:58

billyvg requested a review from a team as a code owner April 10, 2025 19:58

billyvg requested a review from gggritso April 10, 2025 19:58

gggritso reviewed Apr 10, 2025

View reviewed changes

billyvg mentioned this pull request Apr 10, 2025

ref(insights): Refactor cache- + queues- page widgets #89344

Merged

billyvg commented Apr 11, 2025

View reviewed changes

static/app/views/insights/http/views/httpDomainSummaryPage.tsx

Copy link

Member Author

billyvg Apr 11, 2025

Choose a reason for hiding this comment

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

lol Cursor decided to rewrite this file?????????????????????

vercel bot deployed to Preview April 11, 2025 14:19 View deployment

billyvg added 2 commits April 11, 2025 10:20

rename to include Chart

22e96bc

update DomainSummaryPage

646513f

billyvg force-pushed the ref-insights-combine-data-fetching-with-chart-rendering branch from 54bf4ae to 646513f Compare April 11, 2025 14:22

update domain summary hook

b9d30f8

vercel bot deployed to Preview April 11, 2025 14:31 View deployment

Merge branch 'master' into ref-insights-combine-data-fetching-with-ch…

ebff3da

…art-rendering

vercel bot deployed to Preview April 11, 2025 15:08 View deployment

gggritso approved these changes Apr 11, 2025

View reviewed changes

fix lint errors

fb21801

vercel bot deployed to Preview April 11, 2025 16:48 View deployment

billyvg merged commit 56d8dad into master Apr 11, 2025
41 checks passed

billyvg deleted the ref-insights-combine-data-fetching-with-chart-rendering branch April 11, 2025 17:05

DominikB2014 mentioned this pull request Apr 14, 2025

fix(insights): sort not working http landing page #89486

Merged

gggritso added a commit that referenced this pull request Apr 17, 2025

ref(insights): Refactor cache- + queues- page widgets (#89344)

9ccddcd

Part of #88560 Follow-up to #89325 --------- Co-authored-by: George Gritsouk <989898+gggritso@users.noreply.github.com>

andrewshie-sentry pushed a commit that referenced this pull request Apr 22, 2025

ref(insights): Refactor cache- + queues- page widgets (#89344)

5a75b4f

Part of #88560 Follow-up to #89325 --------- Co-authored-by: George Gritsouk <989898+gggritso@users.noreply.github.com>

github-actions bot locked and limited conversation to collaborators Apr 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ref(insights): Combine data loading + widget rendering for Insights -> Http pages #89325

ref(insights): Combine data loading + widget rendering for Insights -> Http pages #89325

Uh oh!

billyvg commented Apr 10, 2025 •

edited

Loading

Uh oh!

billyvg Apr 10, 2025

Uh oh!

gggritso Apr 10, 2025

Uh oh!

Uh oh!

billyvg Apr 10, 2025

Uh oh!

gggritso Apr 10, 2025

Uh oh!

billyvg Apr 11, 2025

Uh oh!

billyvg Apr 10, 2025

Uh oh!

billyvg Apr 10, 2025

Uh oh!

gggritso left a comment

Uh oh!

gggritso Apr 10, 2025

Uh oh!

gggritso Apr 10, 2025

Uh oh!

Uh oh!

billyvg Apr 11, 2025

Uh oh!

billyvg commented Apr 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ref(insights): Combine data loading + widget rendering for Insights -> Http pages #89325

ref(insights): Combine data loading + widget rendering for Insights -> Http pages #89325

Uh oh!

Conversation

billyvg commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggritso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billyvg commented Apr 11, 2025

Uh oh!

Uh oh!

Uh oh!

billyvg commented Apr 10, 2025 •

edited

Loading