[CORE-41] Add new Consolidated Spend Report page #5192

calypsomatic · 2024-12-05T19:49:21Z

Jira Ticket: https://broadworkbench.atlassian.net/browse/CORE-41

Summary of changes:

What

Adds a new Consolidated Spend Report page using the new cross billing spend report API in rawls. This page is hidden behind a feature flag, but a link to it will appear on the billing page if the flag is enabled.
Workspaces that the user has owner permission to will appear, initially sorted by total spend. They can subsequently be sorted by any column, but see note. Spend report can be chosen for the last 7, 30, or 90 days. Results can be searched by workspace name, billing project name, or bucket name.
Results of the report are cached (per time period) so that reloading the page will not make another call to big query (until the next day).

Note: The query is set up to be paginated but as currently implemented, this page requests 250 workspaces (this will be the top spending 250 workspaces), shows them all, and does not allow paginating. I had initially included paginating but waffled over how to implement it and so have removed it until settling on something satisfactory. Mostly the issue is not knowing the total number of workspaces out of which we are displaying. (e.g. "1-10 out of 1000" shown at the bottom of the page). This can be approximately calculated because we actually have ALL user workspaces available on the page - we could filter that for owner access level (this will probably be the same, but not necessarily since the workspaces in the query are determined slightly differently). Alternatively, we can forego a paginator and just load more workspaces as the user scrolls. What is a reasonable number of workspaces to have as a default page size? Or a reasonable max number to put in for pagesize in the query (which does require the pagesize parameter)?

Second note: Sonarcloud is complaining about my use of <role = "row"> and the like instead of <tr>; I have used the same structure elsewhere on the billing page and axe devtools doesn't give me any complaints. My attempts to change the code as suggested by Sonarcloud resulted in absolute disarray on the page. Does anyone have more knowledge or insight into this issue and how much attention I should pay to Sonarcloud?

Why

Testing strategy

calypsomatic · 2024-12-12T21:16:51Z

src/billing/ConsolidatedSpendReport.tsx

+        <div role='columnheader' aria-sort={ariaSort(sort, 'totalStorage')} style={{ flex: 1 }}>
+          <HeaderRenderer sort={sort} onSort={onSort} name='totalStorage' />
+        </div>
+        {/* <div role='columnheader' aria-sort={ariaSort(sort, 'otherSpend')} style={{ flex: 1 }}>


Note: other spend was requested in the query, but left out of the mockup.

src/billing/ConsolidatedSpendReport.tsx

calypsomatic · 2024-12-12T21:19:53Z

src/billing/List/BillingList.tsx

+              type: 'consolidatedSpendReport',
+            })}`}
+            aria-current={false}
+            onClick={void Metrics().captureEvent(Events.billingViewConsolidatedSpendReport)}


The mixpanel event is captured only when the user clicks on the link, so doesn't count things like logging out and then logging back in still on this page or refreshing. I think that should be correct, since there is currently no other way to get to this page, but if there ever appears another link here then we might want to update the event to happen on loading the page.

kevinmarete

This is great, maybe one observation is to make this spinner not to overlay the entire page.

davidangb

Something is funky with sorting the report:

Screen.Recording.17-12-2024.at.15.41.mp4

marctalbott

I'm not having the same sorting issues that David reported

marctalbott · 2024-12-17T21:16:51Z

src/billing/ConsolidatedSpendReport.tsx

+
+// Since the variable names don't match display names for these two columns, we need to map them
+const columnTitleToVariableNameMap = {
+  billingAccount: 'namespace',


This should map to billingProject here and throughout the consolidated report. We tend to reserve "account" for when we're talking about the Google billing account

Meaning just that the words the user should see should be 'Billing Project' instead of 'Billing Account'?

marctalbott · 2024-12-17T21:29:10Z

src/billing/ConsolidatedSpendReport.tsx

+                        billingProject={{
+                          cloudPlatform: 'GCP',
+                          billingAccount: workspace.billingAccount,
+                          projectName: workspace.googleProject,


billingProject.projectName should be workspace.namespace. The Google project is a separate entity.

marctalbott · 2024-12-17T21:32:31Z

src/billing/ConsolidatedSpendReport.tsx

+      <div role='row' style={{ ...Style.cardList.longCardShadowless, padding: 0, flexDirection: 'column' }}>
+        <div style={workspaceCardStyles.row}>
+          <div role='cell' style={workspaceCardStyles.field}>
+            {billingAccountDisplayName ?? '...'}


It might be nice to link to the billing project from the table as well like what we're doing with the workspace dashboard

calypsomatic · 2024-12-18T15:25:57Z

@davidangb

Something is funky with sorting the report:

Screen.Recording.17-12-2024.at.15.41.mp4

It's not sorting, but some weird thing with a workspace somehow duplicating itself. I saw that when I was developing and couldn't figure it out; it was always the same workspace so I just deleted the workspace! I think it was in the same davidan-bp-1 billing project though; I wonder if there's something weird with that bp?

src/billing/ConsolidatedSpendReport.tsx

marctalbott · 2024-12-18T17:02:05Z

src/billing/ConsolidatedSpendReport.tsx

                        billingProject={{
                          cloudPlatform: 'GCP',
-                          billingAccount: workspace.billingAccount,
+                          billingAccount: workspace.namespace,


This was okay as workspace.billingAccount actually. It's the projectName below that should be workspace.namespace. It doesn't really impact this page at all, but the metrics it emits will be wrong.

The billing project is equivalent to the workspace namespace. The billing account refers to the Google billing account that is associated with a given billing project and which gets attached to each workspace's Google project. projectName here refers to the billing project's name, not the workspace's Google project.

marctalbott · 2024-12-18T17:03:42Z

src/libs/feature-previews-config.ts

@@ -160,6 +161,16 @@ const featurePreviewsConfig: readonly FeaturePreview[] = [
    )}`,
    lastUpdated: '12/12/2024',
  },
+  {
+    id: CONSOLIDATED_SPEND_REPORT,
+    title: 'Show spend report for all workspaces owned by user',


Should this be Consolidated Spend Report so users know what to look for in the UI and the messaging is a little more consistent?

davidangb · 2024-12-18T17:05:31Z

It's not sorting, but some weird thing ... I wonder if there's something weird with that bp?

If this has happened to both of us, I fear it could happen to end users as well … I think it's worth tracking down what is causing this!

calypsomatic · 2024-12-18T21:08:52Z

@davidangb

Something is funky with sorting the report:
Screen.Recording.17-12-2024.at.15.41.mp4

It's not sorting, but some weird thing with a workspace somehow duplicating itself. I saw that when I was developing and couldn't figure it out; it was always the same workspace so I just deleted the workspace! I think it was in the same davidan-bp-1 billing project though; I wonder if there's something weird with that bp?

So the issue is that the BigQuery query actually returns the same workspace twice; one with the namespace/name of the workspace, and one without (a null value). The two results share the same google project but have different costs (in the same category, so it's not two different spend categories). Since the identifier for the object in the front end that represents the workspace is based on the namespace/name, we end up with two identical keys and react freaks out when that happens. I haven't been able to figure out why BQ is returning these weird results; if it's something insane due to them being dev workspaces we might be able to leave it as is, otherwise we should modify the backend to consolidate BQ results before sending it to the front end.

davidangb

A few comments inline; nothing major. Are you intending to release this without pagination, or will that eventually be part of this PR?

UX feedback: I find the link to the consolidated spend report to be just kinda hanging out on the page and unappealing. Dunno if anyone has good suggestions for visuals here:

I anticipate that users will very quickly ask, "how can I download the spend report?"

src/billing/ConsolidatedSpendReport.tsx

davidangb · 2025-01-08T17:39:14Z

src/billing/ConsolidatedSpendReport.tsx

+  });
+  const [allWorkspaces, setAllWorkspaces] = useState<WorkspaceWrapper[]>(props.workspaces);
+
+  // TODO - how to know how many workspaces there are  in total in order to paginate without querying BQ?


I see two solutions:

ask BQ for the exact number. This would probably have to be a brand new count() query.

don't attempt to show the exact number; show "results 1-250 of approximately 6500" or the like, and handle errors if a user tries to navigate to a page where there aren't any results. In this approximate mode, you could request 251 rows when you want to display 250; if the backend returns 250 or lower, you know there is no next page; if the backend returns 251, you know there is a next page.

src/billing/ConsolidatedSpendReport.tsx

davidangb · 2025-01-08T17:49:38Z

src/billing/ConsolidatedSpendReport.tsx

+          });
+        } catch {
+          // Return default values for each workspace in case of an error
+          return ownedWorkspaces.map(setDefaultSpendValues);


it would be nice to make the try/catch more granular here. Any errors retrieving the spend report could be handled differently than an error adding spend details to a workspace. Ideally, you could have a try/catch within the loop over the spendDataItems, so a single failure during mapping doesn't cause the entire list to be invalid.

I wouldn't block the PR on this; this could be a future enhancement

davidangb · 2025-01-08T17:51:54Z

src/libs/feature-previews-config.ts

+      'Feedback on Consolidated Spend Report'
+    )}`,
+    lastUpdated: '12/20/2024',
+  },


does this feature preview have an articleUrl?

This shouldn't be its own feature flag. It should be part of the existing "Improved Spend Reports" feature flag.

calypsomatic · 2025-01-08T21:00:39Z

A few comments inline; nothing major. Are you intending to release this without pagination, or will that eventually be part of this PR?

UX feedback: I find the link to the consolidated spend report to be just kinda hanging out on the page and unappealing. Dunno if anyone has good suggestions for visuals here:

I anticipate that users will very quickly ask, "how can I download the spend report?"

I would like to solve pagination before release! That's why I'm hoping to get some opinions or ideas.
Also very happy for UX suggestions on how to dress up the link.

admull · 2025-01-08T21:20:18Z

Also very happy for UX suggestions on how to dress up the link.

There's a mockup in the ticket to have it behave similar to a billing project in the list. It should be its own "card" (although outside of the rest of the list) and have a green indicator once selected. Is that feasible?

admull · 2025-01-08T21:25:18Z

Small sorting visual bug: When sorting by Billing Project or Workspace Name, the arrow does not change direction indicating the sort order. All of the other columns work as expected, only these first two behave incorrectly.

davidangb · 2025-01-09T14:18:39Z

src/billing/ConsolidatedSpendReport.tsx

+
+    const handleSort = (name: string) => {
+      const variableName = columnTitleToVariableNameMap[name];
+      onSort({ field: variableName, direction: sort.direction === 'asc' ? 'desc' : 'asc' });


I think this is backwards:

Suggested change

onSort({ field: variableName, direction: sort.direction === 'asc' ? 'desc' : 'asc' });

onSort({ field: variableName, direction: sort.direction === 'asc' ? 'asc' : 'desc' });

oh, no, that's correct; onSort is the update function so it changes the direction from whatever it was to the other one

sonarqubecloud · 2025-01-10T20:41:50Z

Quality Gate passed

Issues
23 New issues
0 Accepted issues

Measures
0 Security Hotspots
72.1% Coverage on New Code
5.5% Duplication on New Code

See analysis details on SonarQube Cloud

calypsomatic and others added 8 commits December 5, 2024 14:48

skeleton of cbsr

520c4c7

Merge branch 'dev' into core-41-cross-billing

a4cc557

add searching etc

aaa2481

some fixes and renaming

94477d2

udpates and tests

1ec9649

Merge branch 'dev' into core-41-cross-billing

900d0ff

begin caching

8e4acb2

fix test and add mixpanel event

bc30bbb

calypsomatic commented Dec 12, 2024

View reviewed changes

src/billing/ConsolidatedSpendReport.tsx Show resolved Hide resolved

calypsomatic commented Dec 12, 2024

View reviewed changes

calypsomatic and others added 2 commits December 12, 2024 16:30

Merge branch 'dev' into core-41-cross-billing

a35a8e4

hopefully fix test

15a986f

broadbot temporarily deployed to pr-0 December 13, 2024 14:24 Inactive

fix sorting

b837aa6

broadbot temporarily deployed to pr-0 December 13, 2024 15:32 Inactive

calypsomatic and others added 3 commits December 13, 2024 11:19

hopefully fix missing workspaces problem

2348fcf

Merge branch 'dev' into core-41-cross-billing

cfa7271

fix merge

b65972d

broadbot temporarily deployed to pr-0 December 13, 2024 16:42 Inactive

calypsomatic changed the title ~~skeleton of cbsr~~ [CORE-41] Add new Consolidated Spend Report page Dec 13, 2024

update/remove pagination

631c150

broadbot temporarily deployed to pr-0 December 13, 2024 20:28 Inactive

calypsomatic marked this pull request as ready for review December 13, 2024 21:07

calypsomatic requested a review from a team as a code owner December 13, 2024 21:07

calypsomatic requested review from kevinpalis and marctalbott December 13, 2024 21:07

kevinmarete approved these changes Dec 16, 2024

View reviewed changes

davidangb reviewed Dec 17, 2024

View reviewed changes

marctalbott reviewed Dec 17, 2024

View reviewed changes

pr comments

41d6f48

broadbot temporarily deployed to pr-0 December 18, 2024 16:08 Inactive

calypsomatic requested a review from marctalbott December 18, 2024 16:18

Merge branch 'dev' into core-41-cross-billing

4da6ac7

marctalbott reviewed Dec 18, 2024

View reviewed changes

broadbot temporarily deployed to pr-0 December 18, 2024 17:04 Inactive

add new events

4cfa9dc

broadbot temporarily deployed to pr-0 December 18, 2024 21:18 Inactive

Merge branch 'dev' into core-41-cross-billing

a9c27f3

broadbot temporarily deployed to pr-0 January 7, 2025 19:38 Inactive

davidangb reviewed Jan 8, 2025

View reviewed changes

davidangb reviewed Jan 9, 2025

View reviewed changes

begin to updated spend report list item

b0b24ae

broadbot temporarily deployed to pr-0 January 9, 2025 21:46 Inactive

calypsomatic and others added 3 commits January 10, 2025 11:09

update feature preview and hover ui

f52b54d

fix header display

7d43f6c

Merge branch 'dev' into core-41-cross-billing

6bf13e6

broadbot temporarily deployed to pr-0 January 10, 2025 17:09 Inactive

add show all workspaces button

d5ab721

broadbot deployed to pr-0 January 10, 2025 20:42 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-41] Add new Consolidated Spend Report page #5192

[CORE-41] Add new Consolidated Spend Report page #5192

calypsomatic commented Dec 5, 2024 •

edited

Loading

calypsomatic Dec 12, 2024

calypsomatic Dec 12, 2024

kevinmarete left a comment

davidangb left a comment

marctalbott left a comment

marctalbott Dec 17, 2024

calypsomatic Dec 18, 2024

marctalbott Dec 17, 2024

marctalbott Dec 17, 2024

calypsomatic commented Dec 18, 2024

marctalbott Dec 18, 2024

marctalbott Dec 18, 2024

davidangb commented Dec 18, 2024

calypsomatic commented Dec 18, 2024

davidangb left a comment

davidangb Jan 8, 2025

davidangb Jan 8, 2025

davidangb Jan 8, 2025

admull Jan 8, 2025

calypsomatic commented Jan 8, 2025

admull commented Jan 8, 2025

admull commented Jan 8, 2025

davidangb Jan 9, 2025

calypsomatic Jan 9, 2025

calypsomatic Jan 9, 2025

sonarqubecloud bot commented Jan 10, 2025

	onSort({ field: variableName, direction: sort.direction === 'asc' ? 'desc' : 'asc' });
	onSort({ field: variableName, direction: sort.direction === 'asc' ? 'asc' : 'desc' });

[CORE-41] Add new Consolidated Spend Report page #5192

Are you sure you want to change the base?

[CORE-41] Add new Consolidated Spend Report page #5192

Conversation

calypsomatic commented Dec 5, 2024 • edited Loading

Jira Ticket: https://broadworkbench.atlassian.net/browse/CORE-41

Summary of changes:

What

Why

Testing strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinmarete left a comment

Choose a reason for hiding this comment

davidangb left a comment

Choose a reason for hiding this comment

marctalbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calypsomatic commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidangb commented Dec 18, 2024

calypsomatic commented Dec 18, 2024

davidangb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calypsomatic commented Jan 8, 2025

admull commented Jan 8, 2025

admull commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 10, 2025

Quality Gate passed

calypsomatic commented Dec 5, 2024 •

edited

Loading