Repository navigation
feat(gcp): tabbed Google Cloud dashboard with project, region and workload filters - #1496
JeremyFunk wants to merge 7 commits into
Conversation
…kload filters The Google Cloud template put every chart on one page. It now groups them into one tab per service (Cloud Run, Cloud Functions, GKE, Compute Engine, Cloud SQL, Pub/Sub, Load Balancing), fills out the thin tabs, and filters by Project, Region and Workload. Query variables could only list values from traces and logs, which a metrics-only integration never writes. A new `metric` variable source lists a data-point or resource attribute's values from named metrics of one type, read from the raw metric table like the query-builder autocomplete. The option query, the board's provider, the share path, the variables dialog and the v2 API all take it. Template previews now show the root canvas and each section's first tab, since a widget's layout is relative to its tab.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Maple review🟡 Confidence 6/10 · needs attention Reworks the Google Cloud template into one tab per service with project/region/workload variables, and adds a
Findings🟠 Warning · F1 · Project filter substitutes the project id unescaped into SQLsecurity · This template used to build its project clause with 🤖 Prompt to fix this finding with an AI agentWhat was checked
|
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| const PROJECT_FILTER = `resource.cloud.account.id = "$project"` |
There was a problem hiding this comment.
Warning
Project filter substitutes the project id unescaped into SQL
F1 · Warning · security
This template used to build its project clause with escapeMetricStringLiteral(projectId); now the raw project_id template parameter is the project variable's default value (gcp.ts:465) and $project is substituted into the where clause verbatim — interpolateWhereClause replaces a reference with variable.value and does not escape (interpolate.ts:79-85). A project id containing " or \ therefore closes the literal and appends SQL to the metric query instead of returning no rows. Escape the value where it is taken in, as the other templates still do.
Set the default to the escaped value: import `escapeMetricStringLiteral` again and pass `defaultValue: projectId ? escapeMetricStringLiteral(projectId) : undefined`, or keep the clause a build-time literal when `projectId` is set.
🤖 Prompt to fix with an AI agent
In `packages/backend/src/dashboard-templates/infrastructure/gcp.ts:79`: Project filter substitutes the project id unescaped into SQL.
This template used to build its project clause with `escapeMetricStringLiteral(projectId)`; now the raw `project_id` template parameter is the `project` variable's default value (`gcp.ts:465`) and `$project` is substituted into the where clause verbatim — `interpolateWhereClause` replaces a reference with `variable.value` and does not escape (`interpolate.ts:79-85`). A project id containing `"` or `\` therefore closes the literal and appends SQL to the metric query instead of returning no rows. Escape the value where it is taken in, as the other templates still do.
Suggested fix: Set the default to the escaped value: import `escapeMetricStringLiteral` again and pass `defaultValue: projectId ? escapeMetricStringLiteral(projectId) : undefined`, or keep the clause a build-time literal when `projectId` is set.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
…series /lab/gcp-dashboard renders the built template through the read-only canvas: its tabs, the Project / Region / Workload chips over fixture options, and the variables dialog. Series follow the chips through each widget's own where clause, so a filter a widget leaves out (Region on Pub/Sub) changes nothing. Also shortens the dialog's summary of a metric-sourced variable and formats the template files.
Maple review🟡 Confidence 5/10 · needs attention Reworks the Google Cloud template into one tabbed Services section with Project/Region/Workload variables, adding a
Findings🟠 Warning · F2 · Metric names input has no 20-item cap the dashboard schema enforcescorrectness ·
🤖 Prompt to fix this finding with an AI agentStill open from earlier reviews
What was checked
Observability coverage: 2 of 2 changes observable
Files not reviewed (1)The review ended before it read these diffs, so nothing above vouches for them.
|
| source: { | ||
| ...source, | ||
| // Empty entries stay while typing ("a, "); validation flags them. | ||
| metricNames: event.target.value.split(",").map((name) => name.trim()), |
There was a problem hiding this comment.
Warning
Metric names input has no 20-item cap the dashboard schema enforces
F2 · Warning · correctness
V2DashboardVariableSource caps metricNames at 20 entries (packages/domain/src/http/v2/dashboards.ts:545; the widgets schema does the same), but this input splits on every comma with no cap and draftError only rejects empty names. A variable with 21 comma-separated names saves into a document the dashboard schema rejects, so the save fails with a length error that says nothing about the metric count.
Add a `> 20` check for `source.kind === "metric"` to `draftError`, beside the empty-name check at line 112, so the dialog refuses the draft before it reaches the schema.
🤖 Prompt to fix with an AI agent
In `apps/web/src/components/dashboard-builder/config/variables-manager-dialog.tsx:553`: Metric names input has no 20-item cap the dashboard schema enforces.
`V2DashboardVariableSource` caps `metricNames` at 20 entries (`packages/domain/src/http/v2/dashboards.ts:545`; the widgets schema does the same), but this input splits on every comma with no cap and `draftError` only rejects empty names. A variable with 21 comma-separated names saves into a document the dashboard schema rejects, so the save fails with a length error that says nothing about the metric count.
Suggested fix: Add a `> 20` check for `source.kind === "metric"` to `draftError`, beside the empty-name check at line 112, so the dialog refuses the draft before it reaches the schema.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
…o feat/gcp-dashboard-tabs # Conflicts: # packages/backend/src/dashboard-templates/infrastructure/gcp.ts
… into feat/gcp-dashboard-tabs # Conflicts: # apps/web/src/routeTree.gen.ts
…ted on GCP Google Cloud's resource detector sets faas.name on Cloud Run and Cloud Functions, so those services were drawn as AWS Lambda, and GKE services as plain Kubernetes. cloud.provider=gcp now classifies as a gcp platform with the Google Cloud mark, tooltip, colour and legend entry.
…o feat/gcp-dashboard-tabs
Maple review🟡 Confidence 5/10 · needs attention Tabbed Google Cloud dashboard template with Project / Region / Workload query variables over a new
Still open from earlier reviews
What was checked
Observability coverage: 1 of 1 changes observable
|
1 participant
Stacked on #1481.
What
Servicessection, one tab per service: Cloud Run, Cloud Functions (1st gen), GKE, Compute Engine, Cloud SQL, Pub/Sub, Load Balancing.cloud.account.id), Region (cloud.region), Workload (service.name), all with All.project_idtemplate parameter now sets the Project filter's default.New variable source:
metricQuery variables could only list values from traces/logs. Metrics-only integrations never write those.
{ kind: "metric", metricType, metricNames, scope: "metric" | "resource", attributeKey }: values of a data-point or resource attribute on the named metrics.attributeValuestakesmetricNames, and scoped metric discovery readsResourceAttributesfor theresourcescope. Raw-table read, same as the query-builder autocomplete.options.ts, used by the board and by share links), web provider,getMetricAttributeValues, variables dialog (new "Metric attribute…" / "Metric resource attribute…" choices), v2 API schema.Template previews
layout.yis relative to the tab, so the preview now shows the root canvas, then each section's first tab, stacked. The layout test checks rows per container.Verification
us-central1/europe-west1, and the Cloud Run/GKE/VM workloads./lab/gcp-dashboard(new): the built template on the read-only canvas, with fixture series that follow the variable chips through each widget's own where clause. Checked with Playwright: tab switching, Region narrows GKE but leaves Pub/Sub alone, and the variables dialog shows and edits the metric source.Lab
/lab/gcp-dashboardreadsapps/web/src/lab/gcp-dashboard-fixture.json, which isgcpTemplate.build({})(web can't import@maple/backend). The regenerate command is in the lab file.