From 6b354ed594031b1c8c227dcb1e8a8a8be80f0d08 Mon Sep 17 00:00:00 2001 From: Aditya Hegde Date: Tue, 17 Sep 2024 17:50:55 +0530 Subject: [PATCH 1/2] Support multiple measures and dimenisons in alerts --- web-common/src/components/forms/Select.svelte | 16 +++- .../src/features/alerts/BaseAlertForm.svelte | 11 ++- .../features/alerts/CreateAlertForm.svelte | 16 ++-- .../src/features/alerts/alert-preview-data.ts | 2 +- .../alerts/criteria-tab/CriteriaForm.svelte | 18 ++-- .../alerts/criteria-tab/CriteriaGroup.svelte | 2 +- .../alerts/data-tab/AlertDialogDataTab.svelte | 14 +-- .../alerts/extract-alert-form-values.ts | 8 +- web-common/src/features/alerts/form-utils.ts | 85 +++++++++---------- .../routes/(misc)/multi-select/+page.svelte | 12 +++ 10 files changed, 104 insertions(+), 80 deletions(-) create mode 100644 web-local/src/routes/(misc)/multi-select/+page.svelte diff --git a/web-common/src/components/forms/Select.svelte b/web-common/src/components/forms/Select.svelte index 27a5ce42623..27a675e673c 100644 --- a/web-common/src/components/forms/Select.svelte +++ b/web-common/src/components/forms/Select.svelte @@ -6,7 +6,7 @@ const dispatch = createEventDispatcher(); - export let value: string; + export let value: string | string[]; export let id: string; export let label: string; export let options: { @@ -19,8 +19,11 @@ export let optional: boolean = false; export let tooltip: string = ""; export let width: number | null = null; + export let multiple = false; - $: selected = options.find((option) => option.value === value); + $: selected = multiple + ? options.filter((option) => value.includes(option.value)) + : options.find((option) => option.value === value);
@@ -49,10 +52,15 @@ {selected} onSelectedChange={(newSelection) => { if (!newSelection) return; - value = newSelection.value; - dispatch("change", newSelection.value); + if (Array.isArray(newSelection)) { + value = newSelection.map((v) => v.value); + } else { + value = newSelection.value; + } + dispatch("change", value); }} items={options} + {multiple} > (c.measure = mes)); + $: measures = $form.measures; + function measureUpdated(measures: string[]) { + $form.criteria.forEach((c) => { + if (measures.includes(c.measure)) return; + c.measure = measures[0]; + }); } - $: measureUpdated(measure); + $: measureUpdated(measures); diff --git a/web-common/src/features/alerts/CreateAlertForm.svelte b/web-common/src/features/alerts/CreateAlertForm.svelte index c8e20d2e7b1..6d6381dd505 100644 --- a/web-common/src/features/alerts/CreateAlertForm.svelte +++ b/web-common/src/features/alerts/CreateAlertForm.svelte @@ -55,6 +55,13 @@ dimension = $dashboardStore.selectedDimensionName ?? ""; } + let measure = ""; + if ($dashboardStore.tdd.expandedMeasureName) { + measure = $dashboardStore.tdd.expandedMeasureName; + } else if ($dashboardStore.leaderboardMeasureName) { + measure = $dashboardStore.leaderboardMeasureName; + } + // TODO: get metrics view spec const timeRange = mapTimeRange(timeControls, {}); const comparisonTimeRange = mapComparisonTimeRange( @@ -66,16 +73,13 @@ const formState = createForm({ initialValues: { name: "", - measure: - $dashboardStore.tdd.expandedMeasureName ?? - $dashboardStore.leaderboardMeasureName ?? - "", - splitByDimension: dimension, + measures: measure ? [measure] : [], + dimensions: dimension ? [dimension] : [], evaluationInterval: "", criteria: [ { ...getEmptyMeasureFilterEntry(), - measure: $dashboardStore.leaderboardMeasureName ?? "", + measure, }, ], criteriaOperation: V1Operation.OPERATION_AND, diff --git a/web-common/src/features/alerts/alert-preview-data.ts b/web-common/src/features/alerts/alert-preview-data.ts index 7b854d6ba87..fcabf0fc8d5 100644 --- a/web-common/src/features/alerts/alert-preview-data.ts +++ b/web-common/src/features/alerts/alert-preview-data.ts @@ -80,7 +80,7 @@ function getAlertPreviewQueryOptions( > { return { enabled: - !!formValues.measure && + !!formValues.measures.length && !!metricsViewSpec && (!formValues.timeRange || !!formValues.timeRange.end) && (!formValues.comparisonTimeRange || !!formValues.comparisonTimeRange.end), diff --git a/web-common/src/features/alerts/criteria-tab/CriteriaForm.svelte b/web-common/src/features/alerts/criteria-tab/CriteriaForm.svelte index 11c7422f315..13234375c7c 100644 --- a/web-common/src/features/alerts/criteria-tab/CriteriaForm.svelte +++ b/web-common/src/features/alerts/criteria-tab/CriteriaForm.svelte @@ -21,17 +21,13 @@ $form["metricsViewName"], ); - $: measure = $metricsView.data?.measures?.find( - (m) => m.name === $form["measure"], - ); - $: measureOptions = [ - { - value: $form["measure"], - label: measure?.label?.length - ? measure.label - : measure?.expression ?? $form["measure"], - }, - ]; + $: measureOptions = + $metricsView.data?.measures + ?.filter((m) => $form["measures"].includes(m.name ?? "")) + .map((m) => ({ + value: m.name!, + label: m.label?.length ? m.label : m.expression ?? m.name ?? "", + })) ?? []; $: selectedMeasure = $metricsView.data?.measures?.find( (m) => m.name === $form["criteria"][index].measure, ); diff --git a/web-common/src/features/alerts/criteria-tab/CriteriaGroup.svelte b/web-common/src/features/alerts/criteria-tab/CriteriaGroup.svelte index 8590ecf5686..aa79e88b829 100644 --- a/web-common/src/features/alerts/criteria-tab/CriteriaGroup.svelte +++ b/web-common/src/features/alerts/criteria-tab/CriteriaGroup.svelte @@ -19,7 +19,7 @@ function handleAddCriteria() { $form["criteria"] = $form["criteria"].concat({ ...getEmptyMeasureFilterEntry(), - measure: $form.measure, + measure: $form.measures[0], }); } diff --git a/web-common/src/features/alerts/data-tab/AlertDialogDataTab.svelte b/web-common/src/features/alerts/data-tab/AlertDialogDataTab.svelte index 0bfe3ccc119..8949f261ab4 100644 --- a/web-common/src/features/alerts/data-tab/AlertDialogDataTab.svelte +++ b/web-common/src/features/alerts/data-tab/AlertDialogDataTab.svelte @@ -57,19 +57,21 @@ title="Alert data" > ; @@ -66,8 +66,8 @@ export function extractAlertFormValues( ); return { - measure: measures[0]?.name ?? "", - splitByDimension: dimensions[0]?.name ?? "", + measures: measures.map((m) => m.name ?? ""), + dimensions: dimensions.map((d) => d.name ?? ""), criteria: (queryArgs.having?.cond?.exprs?.map( mapExprToMeasureFilter, diff --git a/web-common/src/features/alerts/form-utils.ts b/web-common/src/features/alerts/form-utils.ts index 77ea0e041c5..506738ed8ce 100644 --- a/web-common/src/features/alerts/form-utils.ts +++ b/web-common/src/features/alerts/form-utils.ts @@ -9,8 +9,9 @@ import { MeasureFilterType } from "@rilldata/web-common/features/dashboards/filt import { mergeDimensionAndMeasureFilter } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-utils"; import { sanitiseExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import { DimensionThresholdFilter } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; -import type { +import { V1Expression, + V1MetricsViewAggregationMeasure, V1MetricsViewAggregationRequest, V1Operation, V1TimeRange, @@ -19,8 +20,8 @@ import * as yup from "yup"; export type AlertFormValues = { name: string; - measure: string; - splitByDimension: string; + measures: string[]; + dimensions: string[]; criteria: MeasureFilterEntry[]; criteriaOperation: V1Operation; evaluationInterval: string; @@ -42,38 +43,38 @@ export type AlertFormValues = { export function getAlertQueryArgsFromFormValues( formValues: AlertFormValues, ): V1MetricsViewAggregationRequest { + const measures: V1MetricsViewAggregationMeasure[] = []; + formValues.measures.filter(Boolean).forEach((measure) => { + measures.push({ name: measure }); + if (formValues.comparisonTimeRange) { + measures.push( + { + name: measure + ComparisonDeltaAbsoluteSuffix, + comparisonDelta: { measure }, + }, + { + name: measure + ComparisonDeltaRelativeSuffix, + comparisonRatio: { measure }, + }, + ); + } + if ( + formValues.criteria.some( + (c) => + c.measure === measure && c.type === MeasureFilterType.PercentOfTotal, + ) + ) { + measures.push({ + name: measure + ComparisonPercentOfTotal, + percentOfTotal: { measure }, + }); + } + }); + return { metricsView: formValues.metricsViewName, - measures: [ - { - name: formValues.measure, - }, - ...(formValues.comparisonTimeRange - ? [ - { - name: formValues.measure + ComparisonDeltaAbsoluteSuffix, - comparisonDelta: { measure: formValues.measure }, - }, - { - name: formValues.measure + ComparisonDeltaRelativeSuffix, - comparisonRatio: { measure: formValues.measure }, - }, - ] - : []), - ...(formValues.criteria.some( - (c) => c.type === MeasureFilterType.PercentOfTotal, - ) - ? [ - { - name: formValues.measure + ComparisonPercentOfTotal, - percentOfTotal: { measure: formValues.measure }, - }, - ] - : []), - ], - dimensions: formValues.splitByDimension - ? [{ name: formValues.splitByDimension }] - : [], + measures, + dimensions: formValues.dimensions.map((d) => ({ name: d })), where: sanitiseExpression( mergeDimensionAndMeasureFilter( formValues.whereFilter, @@ -94,12 +95,10 @@ export function getAlertQueryArgsFromFormValues( timeZone: formValues.timeRange.timeZone, roundToGrain: formValues.timeRange.roundToGrain, }, - sort: [ - { - name: formValues.measure, - desc: false, - }, - ], + sort: formValues.measures.filter(Boolean).map((m) => ({ + name: m, + desc: false, + })), ...(formValues.comparisonTimeRange ? { comparisonTimeRange: { @@ -113,7 +112,7 @@ export function getAlertQueryArgsFromFormValues( export const alertFormValidationSchema = yup.object({ name: yup.string().required("Required"), - measure: yup.string().required("Required"), + measures: yup.array().of(yup.string()).min(1), criteria: yup.array().of( yup.object().shape({ measure: yup.string().required("Required"), @@ -150,7 +149,7 @@ export const alertFormValidationSchema = yup.object({ ), }); export const FieldsByTab: (keyof AlertFormValues)[][] = [ - ["measure"], + ["measures"], ["criteria", "criteriaOperation"], ["name", "snooze", "slackUsers", "emailRecipients"], ]; @@ -164,8 +163,8 @@ export function checkIsTabValid( let hasErrors: boolean; if (tabIndex === 0) { - hasRequiredFields = formValues.measure !== ""; - hasErrors = !!errors.measure; + hasRequiredFields = formValues.measures.length > 0; + hasErrors = !!errors.measures; } else if (tabIndex === 1) { hasRequiredFields = true; formValues.criteria.forEach((criteria) => { diff --git a/web-local/src/routes/(misc)/multi-select/+page.svelte b/web-local/src/routes/(misc)/multi-select/+page.svelte new file mode 100644 index 00000000000..29c4f3b97d6 --- /dev/null +++ b/web-local/src/routes/(misc)/multi-select/+page.svelte @@ -0,0 +1,12 @@ + + + From 657979224f4f7be831091e43b5a2776d7e33196a Mon Sep 17 00:00:00 2001 From: Aditya Hegde Date: Tue, 17 Sep 2024 21:25:43 +0530 Subject: [PATCH 2/2] Fix reopening dashboard --- .../getDashboardFromAggregationRequest.ts | 3 + .../dashboards/query-mappers/utils.ts | 69 +++++++++---------- .../alerts/criteria-tab/getTypeOptions.ts | 5 +- web-common/src/features/alerts/utils.spec.ts | 6 +- 4 files changed, 44 insertions(+), 39 deletions(-) diff --git a/web-admin/src/features/dashboards/query-mappers/getDashboardFromAggregationRequest.ts b/web-admin/src/features/dashboards/query-mappers/getDashboardFromAggregationRequest.ts index e0f1d9c4352..b42419c1db7 100644 --- a/web-admin/src/features/dashboards/query-mappers/getDashboardFromAggregationRequest.ts +++ b/web-admin/src/features/dashboards/query-mappers/getDashboardFromAggregationRequest.ts @@ -72,8 +72,11 @@ export async function getDashboardFromAggregationRequest({ if (req.having?.cond?.exprs?.length && req.dimensions?.[0]?.name) { const dimension = req.dimensions[0].name; if ( + // we only support 1-1 map from measure filter to dimension req.having.cond.exprs.length > 1 || + // we do not support comparison based measure filter exprHasComparison(req.having) || + // there is already measure filter from dashboard during alert creation dashboard.dimensionThresholdFilters.length > 0 ) { const expr = await convertExprToToplist( diff --git a/web-admin/src/features/dashboards/query-mappers/utils.ts b/web-admin/src/features/dashboards/query-mappers/utils.ts index 8e497ae911b..15877fc99ed 100644 --- a/web-admin/src/features/dashboards/query-mappers/utils.ts +++ b/web-admin/src/features/dashboards/query-mappers/utils.ts @@ -3,9 +3,11 @@ import { ComparisonDeltaRelativeSuffix, ComparisonPercentOfTotal, } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-entry"; +import { MeasureFilterType } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-options"; import { createInExpression, forEachIdentifier, + getAllIdentifiers, } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; import { PreviousCompleteRangeMap } from "@rilldata/web-common/features/dashboards/time-controls/time-range-mappers"; @@ -20,6 +22,7 @@ import { queryServiceMetricsViewAggregation, type QueryServiceMetricsViewAggregationBody, type V1Expression, + V1MetricsViewAggregationMeasure, type V1TimeRange, type V1TimeRangeSummary, } from "@rilldata/web-common/runtime-client"; @@ -116,48 +119,46 @@ export async function convertExprToToplist( queryClient: QueryClient, instanceId: string, metricsView: string, - dimensionName: string, - measureName: string, + dimensionNames: string[], + measureNames: string[], timeRange: V1TimeRange | undefined, comparisonTimeRange: V1TimeRange | undefined, executionTime: string, where: V1Expression | undefined, having: V1Expression, ) { - let hasPercentOfTotals = false; + const havingIdentifiers = getAllIdentifiers(having); forEachIdentifier(having, (_, ident) => { if (ident?.endsWith(ComparisonPercentOfTotal)) { hasPercentOfTotals = true; } }); + const measures: V1MetricsViewAggregationMeasure[] = []; + measureNames.forEach((measure) => { + measures.push({ name: measure }); + if (comparisonTimeRange) { + measures.push( + { + name: measure + ComparisonDeltaAbsoluteSuffix, + comparisonDelta: { measure }, + }, + { + name: measure + ComparisonDeltaRelativeSuffix, + comparisonRatio: { measure }, + }, + ); + } + if (havingIdentifiers.includes(measure + ComparisonPercentOfTotal)) { + measures.push({ + name: measure + ComparisonPercentOfTotal, + percentOfTotal: { measure }, + }); + } + }); const toplistBody: QueryServiceMetricsViewAggregationBody = { - measures: [ - { - name: measureName, - }, - ...(comparisonTimeRange - ? [ - { - name: measureName + ComparisonDeltaAbsoluteSuffix, - comparisonDelta: { measure: measureName }, - }, - { - name: measureName + ComparisonDeltaRelativeSuffix, - comparisonRatio: { measure: measureName }, - }, - ] - : []), - ...(hasPercentOfTotals - ? [ - { - name: measureName + ComparisonPercentOfTotal, - percentOfTotal: { measure: measureName }, - }, - ] - : []), - ], - dimensions: [{ name: dimensionName }], + measures, + dimensions: dimensionNames.map((d) => ({ name: d })), ...(timeRange ? { timeRange: { @@ -176,12 +177,10 @@ export async function convertExprToToplist( : {}), where, having, - sort: [ - { - name: measureName, - desc: false, - }, - ], + sort: measures.map((m) => ({ + name: m, + desc: false, + })), limit: "250", }; const toplist = await queryClient.fetchQuery({ diff --git a/web-common/src/features/alerts/criteria-tab/getTypeOptions.ts b/web-common/src/features/alerts/criteria-tab/getTypeOptions.ts index 4feee0fb741..8241e7e391c 100644 --- a/web-common/src/features/alerts/criteria-tab/getTypeOptions.ts +++ b/web-common/src/features/alerts/criteria-tab/getTypeOptions.ts @@ -46,7 +46,10 @@ export function getTypeOptions( ); } - if (selectedMeasure?.validPercentOfTotal && formValues.splitByDimension) { + if ( + selectedMeasure?.validPercentOfTotal && + formValues.dimensions.length > 0 + ) { options.push(MeasureFilterPercentOfTotalOption); } else { options.push({ diff --git a/web-common/src/features/alerts/utils.spec.ts b/web-common/src/features/alerts/utils.spec.ts index 136840c3a43..240f46a7c30 100644 --- a/web-common/src/features/alerts/utils.spec.ts +++ b/web-common/src/features/alerts/utils.spec.ts @@ -25,7 +25,7 @@ describe("generateAlertName", () => { { title: "value criteria without comparison", formValues: { - measure: "total_records", + measures: ["total_records"], criteria: [ { measure: "total_records", @@ -41,7 +41,7 @@ describe("generateAlertName", () => { { title: "absolute change criteria with comparison", formValues: { - measure: "total_records", + measures: ["total_records"], criteria: [ { measure: "total_records", @@ -60,7 +60,7 @@ describe("generateAlertName", () => { { title: "percent change criteria with comparison", formValues: { - measure: "total_records", + measures: ["total_records"], criteria: [ { measure: "total_records",