Skip to content
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.

Sign up for GitHub

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

fix: public urls with computed measures and dependant measures are hidden #6520

Merged
merged 6 commits into from
Jan 30, 2025
Merged

fix: public urls with computed measures and dependant measures are hidden #6520

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d2c104d
Add requiredMeasures to explore fields
AdityaHegde Jan 29, 2025
f35edf4
Do not send referenced measures for non-window measures
AdityaHegde Jan 29, 2025
6eb63af
Fix windowed measures
AdityaHegde Jan 29, 2025
e6a6249
Fix lint+tests
AdityaHegde Jan 29, 2025
ca1e48b
Refactor filter method
AdityaHegde Jan 30, 2025
5dc3a46
Change function name
AdityaHegde Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions web-common/src/features/dashboards/dimension-table/DimensionDisplay.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
type MetricsViewSpecDimensionV2,
type V1Expression,
type V1MetricsViewAggregationMeasure,
type V1MetricsViewSpec,
type V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import { runtime } from "@rilldata/web-common/runtime-client/runtime-store";
Expand All @@ -36,7 +35,6 @@
export let whereFilter: V1Expression;
export let metricsViewName: string;
export let dimensionThresholdFilters: DimensionThresholdFilter[];
export let metricsView: V1MetricsViewSpec;
export let visibleMeasureNames: string[];
export let timeControlsReady: boolean;
export let dimension: MetricsViewSpecDimensionV2;
Expand Down Expand Up @@ -102,7 +100,6 @@
$: measures = getMeasuresForDimensionTable(
activeMeasureName,
dimensionThresholdFilters,
metricsView,
visibleMeasureNames,
)
.map(
Expand Down
8 changes: 1 addition & 7 deletions web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
MetricsViewSpecDimensionV2,
V1Expression,
V1MetricsViewAggregationMeasure,
V1MetricsViewSpec,
V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import {
Expand All @@ -24,7 +23,6 @@
additionalMeasures,
getFiltersForOtherDimensions,
} from "../selectors";
import { getIndependentMeasures } from "../state-managers/selectors/measures";
import {
createAndExpression,
createOrExpression,
Expand Down Expand Up @@ -60,7 +58,6 @@
export let dimensionThresholdFilters: DimensionThresholdFilter[];
export let activeMeasureName: string;
export let metricsViewName: string;
export let metricsView: V1MetricsViewSpec;
export let sortType: SortType;
export let tableWidth: number;
export let sortedAscending: boolean;
Expand Down Expand Up @@ -125,10 +122,7 @@
undefined,
);

$: measures = getIndependentMeasures(
metricsView,
additionalMeasures(activeMeasureName, dimensionThresholdFilters),
)
$: measures = additionalMeasures(activeMeasureName, dimensionThresholdFilters)
.map(
(n) =>
({
Expand Down
3 changes: 0 additions & 3 deletions web-common/src/features/dashboards/leaderboard/LeaderboardDisplay.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import { getStateManagers } from "@rilldata/web-common/features/dashboards/state-managers/state-managers";
import type {
V1Expression,
V1MetricsViewSpec,
V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import { runtime } from "@rilldata/web-common/runtime-client/runtime-store";
Expand All @@ -22,7 +21,6 @@
export let comparisonTimeRange: V1TimeRange | undefined;
export let timeControlsReady: boolean;
export let activeMeasureName: string;
export let metricsView: V1MetricsViewSpec;

const StateManagers = getStateManagers();
const {
Expand Down Expand Up @@ -108,7 +106,6 @@
isSummableMeasure={$isSummableMeasure}
{parentElement}
{suppressTooltip}
{metricsView}
{timeControlsReady}
selectedValues={$selectedDimensionValues(dimension.name)}
isBeingCompared={$isBeingComparedReadable(dimension.name)}
Expand Down
5 changes: 1 addition & 4 deletions web-common/src/features/dashboards/state-managers/selectors/dashboard-queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
import { getIndependentMeasures } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import type { V1MetricsViewSpec } from "@rilldata/web-common/runtime-client";
import { additionalMeasures } from "../../selectors";
import type { DimensionThresholdFilter } from "../../stores/metrics-explorer-entity";

export function getMeasuresForDimensionTable(
activeMeasureName: string,
dimensionThresholdFilters: DimensionThresholdFilter[],
metricsView: V1MetricsViewSpec | undefined,
visibleMeasureNames: string[],
) {
const allMeasures = new Set([
...visibleMeasureNames,
...additionalMeasures(activeMeasureName, dimensionThresholdFilters),
]);
return getIndependentMeasures(metricsView ?? {}, [...allMeasures]);
return [...allMeasures];
}
8 changes: 8 additions & 0 deletions web-common/src/features/dashboards/state-managers/selectors/measures.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ describe("measures selectors", () => {
timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
expected: {
measures: ["mes", "mes_time_no_grain"],
nonWindowMeasures: ["mes", "mes_time_no_grain"],
dimensions: [
{
name: "time",
Expand All @@ -34,6 +35,7 @@ describe("measures selectors", () => {
timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
expected: {
measures: ["mes", "mes_time_no_grain", "mes_time_day_grain"],
nonWindowMeasures: ["mes", "mes_time_no_grain", "mes_time_day_grain"],
dimensions: [
{
name: "time",
Expand All @@ -53,6 +55,11 @@ describe("measures selectors", () => {
timeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
expected: {
measures: ["mes", "mes_time_no_grain", "mes_time_week_grain"],
nonWindowMeasures: [
"mes",
"mes_time_no_grain",
"mes_time_week_grain",
],
dimensions: [
{
name: "time",
Expand All @@ -72,6 +79,7 @@ describe("measures selectors", () => {
timeGrain: V1TimeGrain.TIME_GRAIN_MONTH,
expected: {
measures: ["mes", "mes_time_no_grain"],
nonWindowMeasures: ["mes", "mes_time_no_grain"],
dimensions: [
{
name: "time",
Expand Down
23 changes: 4 additions & 19 deletions web-common/src/features/dashboards/state-managers/selectors/measures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,12 @@ export const getFilteredMeasuresAndDimensions = ({
measureNames: string[],
): {
measures: string[];
nonWindowMeasures: string[];
dimensions: MetricsViewSpecDimensionSelector[];
} => {
const dimensions = new Map<string, V1TimeGrain>();
const measures = new Set<string>();
const nonWindowMeasures = new Set<string>();
measureNames.forEach((measureName) => {
const measure = metricsViewSpec.measures?.find(
(m) => m.name === measureName,
Expand Down Expand Up @@ -145,10 +147,11 @@ export const getFilteredMeasuresAndDimensions = ({
});
if (skipMeasure) return;
measures.add(measureName);
measure.referencedMeasures?.filter((refMes) => measures.add(refMes));
if (!measure.window) nonWindowMeasures.add(measureName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble following this function.

  1. What is a "filtered" measure and dimension?
  2. I'm nervous about this line. Adding measures to the list of user-selected measures feels like exactly what caused the original bug. Double-checking... are you sure this is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding these measures break the totals query, since we dont really get totals data anyway (no data can be seen) it is better to filter them out.

Would be great to add E2E once the setup PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems one way this function is overloaded is that it's preparing two different query requests: totals and timeseries, which each have a different set of measures & dimensions.

Can we split this into two functions:

  1. getMeasuresAndDimensionsForTimeSeriesRequest()
  2. getMeasuresForTotalsRequest()

Copy link
Collaborator Author

@AdityaHegde AdityaHegde Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Updated the method, added a param to include/exclude window measures. I dont think the method should differentiate between timeseries vs totals request.

});
return {
measures: [...measures],
nonWindowMeasures: [...nonWindowMeasures],
dimensions: [...dimensions.entries()].map(([name, timeGrain]) => ({
name,
timeGrain,
Expand All @@ -157,24 +160,6 @@ export const getFilteredMeasuresAndDimensions = ({
};
};

export const getIndependentMeasures = (
metricsViewSpec: V1MetricsViewSpec,
measureNames: string[],
) => {
const measures = new Set<string>();
measureNames.forEach((measureName) => {
const measure = metricsViewSpec.measures?.find(
(m) => m.name === measureName,
);
// temporary check for window measures until the PR to move to AggregationApi is merged
if (!measure || measure.requiredDimensions?.length || !!measure.window)
return;
measures.add(measureName);
measure.referencedMeasures?.filter((refMes) => measures.add(refMes));
});
return [...measures];
};

export const getSimpleMeasures = (measures: MetricsViewSpecMeasureV2[]) => {
return (
measures?.filter(
Expand Down
38 changes: 13 additions & 25 deletions web-common/src/features/dashboards/time-series/timeseries-data-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import { mergeMeasureFilters } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-utils";
import {
getFilteredMeasuresAndDimensions,
getIndependentMeasures,
} from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import { getFilteredMeasuresAndDimensions } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import type { StateManagers } from "@rilldata/web-common/features/dashboards/state-managers/state-managers";
import { sanitiseExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils";
import { useTimeControlStore } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store";
Expand Down Expand Up @@ -133,16 +130,13 @@ export function createTimeSeriesDataStore(
: [];
}

const { measures: filteredMeasures } = getFilteredMeasuresAndDimensions({
dashboard: dashboardStore,
})(metricsView ?? {}, measures);
const independentMeasures = getIndependentMeasures(
metricsView ?? {},
measures,
);
const { measures: filteredMeasures, nonWindowMeasures } =
getFilteredMeasuresAndDimensions({
dashboard: dashboardStore,
})(metricsView ?? {}, measures);

const primaryTimeSeries =
measures.length > 0
filteredMeasures.length > 0
? createMetricsViewTimeSeries(ctx, filteredMeasures, false)
: writable({
isFetching: false,
Expand All @@ -152,13 +146,13 @@ export function createTimeSeriesDataStore(
});

const primaryTotals =
measures.length > 0
? createTotalsForMeasure(ctx, independentMeasures, false)
nonWindowMeasures.length > 0
? createTotalsForMeasure(ctx, nonWindowMeasures, false)
: writable({
isFetching: false,
isError: false,
data: null,
error: {},
data: { data: [] },
error: undefined,
});

let unfilteredTotals:
Expand All @@ -168,7 +162,7 @@ export function createTimeSeriesDataStore(
if (dashboardStore?.selectedComparisonDimension) {
unfilteredTotals = createUnfilteredTotalsForMeasure(
ctx,
independentMeasures,
measures,
dashboardStore?.selectedComparisonDimension,
);
}
Expand All @@ -184,11 +178,7 @@ export function createTimeSeriesDataStore(
filteredMeasures,
true,
);
comparisonTotals = createTotalsForMeasure(
ctx,
independentMeasures,
true,
);
comparisonTotals = createTotalsForMeasure(ctx, measures, true);
}

let dimensionTimeSeriesCharts:
Expand Down Expand Up @@ -242,9 +232,7 @@ export function createTimeSeriesDataStore(
}
if (primaryTotal.error) {
isError = true;
error["totals"] = (
primaryTotal.error as HTTPError
).response?.data?.message;
error["totals"] = primaryTotal.error.response?.data?.message;
}

return {
Expand Down
4 changes: 0 additions & 4 deletions web-common/src/features/dashboards/workspace/Dashboard.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@
}
: undefined;
$: metricsView = $explore.data?.metricsView ?? {};
let metricsWidth = DEFAULT_TIMESERIES_WIDTH;
let resizing = false;
</script>
Expand Down Expand Up @@ -187,7 +185,6 @@
{comparisonTimeRange}
activeMeasureName={$activeMeasureName}
{timeControlsReady}
{metricsView}
visibleMeasureNames={$visibleMeasures.map(
({ name }) => name ?? "",
)}
Expand All @@ -201,7 +198,6 @@
{dimensionThresholdFilters}
{timeRange}
{comparisonTimeRange}
{metricsView}
{timeControlsReady}
/>
{/if}
Expand Down
Loading