Conversation
nishantmonu51
added
Type:Feature
AdityaHegde
left a comment
AdityaHegde
left a comment
There was a problem hiding this comment.
Timeseries chart icon is squished. How about showing it in top left?
Big number icon is not aligned. Missing items-center somewhere.
Note that we are not showing any info in other screens like pivot and time dimension detail. Not sure if the customer asking for this use those. cc @nishantmonu51
| // The widget is missing data from max(requested start, widest coverage start) up to | ||
| // its own coverage start. Only warn when the gap exceeds one grain bucket of the | ||
| // widest table (grain-truncation tolerance; 0 for the base table). | ||
| const availableStartMs = Math.max(requestedStartMs, widestMin); |
There was a problem hiding this comment.
What case do we need to check with widest table vs just the requested range? We can simplify a lot if we only check with requested range.
| "dashboard_output_excludes": "Output excludes selected values", | ||
| "dashboard_output_includes": "Output includes selected values", | ||
| "dashboard_partial_data": "Partial data", | ||
| "dashboard_partial_data_detail": "This view only includes data from {servedStart} because the fields it uses have limited history. Other parts of this dashboard include data from {availableStart}.", |
There was a problem hiding this comment.
Other parts... is already part of time range controls, feels too much info for the tooltip, epecially since the user will be aware of selected time range.
| /** Rollup table name → time grain, for grain-truncation tolerance. */ | ||
| readonly rollupGrains: Readable<Map<string, V1TimeGrain>>; | ||
| /** Serving tables of all widgets currently on the dashboard. */ | ||
| readonly tablesInUse: Readable<Set<string>>; |
There was a problem hiding this comment.
Why is this needed? Shouldnt each widget be aware of just the start/end of selected time range, table queried and its available start/end?
|
|
||
| export interface CoverageWarning { | ||
| /** Timestamp (ms) where the widget's serving table's data actually starts. */ | ||
| servedStartMs: number; |
There was a problem hiding this comment.
Are we not planning to support when end doesnt match?
| }, | ||
| ); | ||
|
|
||
| $: chartServingTable = servingTableOf($timeSeriesQuery.data); |
There was a problem hiding this comment.
How about simplifying components with a utility,
// RollupCoverageStore.ts
this.computeCoverageWarning = $derived([this.coverage, this.rollupGrains, this.tablesInUse], () => {
return (data: { servingTable?: string } | undefined, requestedStartMs: number) => {
// does calc similar to existing servingTableOf + computeCoverageWarning
}
});
// MeasureChart.svelte
const computeCoverageWarning = rollupCoverage?.computeCoverageWarning ?? readable(undefined)
$: primaryCoverageWarning = $computeCoverageWarning?.($timeSeriesQuery.data, requestedStartMs(timeStart))
$: comparisonCoverageWarning = ...
$: coverageWarning = primaryCoverageWarning ?? comparisonCoverageWarning
| import { DateTime } from "luxon"; | ||
| import type { CoverageWarning } from "./rollup-coverage"; | ||
|
|
||
| export let warning: CoverageWarning; |
There was a problem hiding this comment.
New component should be in svelte5.
|
@nishantmonu51 is of opinion that we should not do this, we should assume rollups are always subsets of base table. We should document this and still if customer does this then not then we should not handle the error cases and its their responsibility. |
| // Rollup table the query was routed to. Empty when the query was served from the metrics view's base table. | ||
| // Matches the keys of rollup_time_ranges in MetricsViewTimeRangeResponse and MetricsViewTimeRangesResponse. | ||
| string serving_table = 4; |
There was a problem hiding this comment.
It maybe feels a little dirty to use table names here since a) usually we advise using models, not tables, b) it ignores database/schema name so could theoretically have collisions, c) leaks underlying table info into metrics queries.
It's not a huge problem, so you can keep if you think it makes sense, but otherwise consider instead either a) using index in spec.rollups, or b) fully qualified table names.
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
The PR thread still records an open product question: whether a rollup may ever hold wider retention than its base table, or whether rollups should simply be documented as subsets with mismatches left to the customer. That decision determines whether this warning is needed at all, and it has not been settled on the thread.
Separately, the branch is behind main and currently conflicts in MeasureBigNumber.svelte, DimensionDisplay.svelte, DimensionHeader.svelte, Leaderboard.svelte, LeaderboardDisplay.svelte, LeaderboardHeader.svelte, and the generated queries.pb.go. The new proto field numbers do not collide with anything added on main, so the rebase plus make proto.generate should be mechanical.
| const updatePrimaryServingTable = rollupCoverage | ||
| ? registerWidgetServingTable(rollupCoverage) | ||
| : undefined; | ||
| const updateComparisonServingTable = rollupCoverage | ||
| ? registerWidgetServingTable(rollupCoverage) | ||
| : undefined; | ||
| const coverageStore = rollupCoverage?.coverage ?? readable(undefined); | ||
| const tablesInUseStore = | ||
| rollupCoverage?.tablesInUse ?? readable(new Set<string>()); | ||
| const rollupGrainsStore = | ||
| rollupCoverage?.rollupGrains ?? readable(new Map<string, V1TimeGrain>()); |
There was a problem hiding this comment.
This wiring is repeated verbatim in Leaderboard.svelte:111, MeasureChart.svelte:87, and DimensionDisplay.svelte:78: the conditional registerWidgetServingTable, the readable(...) null-object fallbacks for the optional rollupCoverage prop, and the computeCoverageWarning(...) reactive block, with the chart and this file additionally duplicating the primary/comparison "first warning wins" chain. Any change to the warning inputs, such as the end-side checks raised inline, has to be made in four places and will drift. A derived helper on RollupCoverageStore that closes over coverage, tablesInUse, and rollupGrains and returns (data, requestedStartMs) => CoverageWarning | undefined, plus a shared no-op instance for callers without a dashboard, would collapse each widget to one or two lines and remove the readable(undefined) fallbacks entirely.
| * widget id changes and deregistration on component destroy. Must be called during | ||
| * component initialization. | ||
| */ | ||
| export function registerWidgetServingTable( |
There was a problem hiding this comment.
registerWidgetServingTable is called from exactly four components: leaderboards, big numbers, measure charts, and the dimension table. The TDD table (via MetricsViewTimeSeries) and the pivot (via MetricsViewAggregation) receive the same servingTable field on their responses but neither render a warning nor contribute to tablesInUse. A base-served TDD table therefore sits under a rollup-served chart with no indicator, and a base-served pivot can suppress a warning on a sibling widget that would otherwise fire. If deferring those two is deliberate for a first iteration, the PR description should say so.
@nishantmonu51 Would be good to decide on this and then move forward |
This branch has not been deployed
4 participants
This is to solve cases when dimensions are served from different tables example one from a rollup another from base because may be that dimension was not present in the rollup or some other criteria. So add a warning in such cases, we return the serving table along with response, UI already know the rollup and base coverage so it can do mismatch check with time coverage of serving table.
Backend changes
Expose which physical table served a metrics query, plus per-rollup time coverage, so the frontend can detect when widgets on one dashboard are served from tables with different retention:
serving_tableadded to MetricsViewAggregationResponse, MetricsViewTimeSeriesResponse, and MetricsViewComparisonResponse: the rollup table the query was routed to, empty when served from the base table.rollup_time_ranges(map<table, TimeRangeSummary>) added to MetricsViewTimeRange and MetricsViewTimeRanges responses.Frontend warning logic
Each dashboard gets a RollupCoverageStore (on StateManagers) holding per-table time coverage from MetricsViewTimeRange (timeRangeSummary + new rollupTimeRanges) and a registry of which table served each mounted widget (from the new servingTable field on query responses; leaderboards, big numbers, charts, and the dimension table register).
A widget shows the warning if its serving table's coverage starts later than both the requested range start (primary or comparison, whichever is earlier) and the widest coverage among all tables currently in use on the dashboard:
served.min > max(requestedStart, widestInUse.min) + oneGrainBucket(widestTable)
The grain-bucket tolerance suppresses false positives from rollup min timestamps being truncated to bucket starts. Consequences: only widgets actually missing requested data warn; if all widgets route to the same table (e.g. a filter forces everything to base), the dashboard is internally consistent and nothing warns; ranges fully covered by the serving table never warn regardless of table mismatch.
Checklist: