Repository navigation
Conversation
Signed-off-by: Nitant28 <singhnitant28@gmail.com>
Signed-off-by: Nitant28 <singhnitant28@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Fixes the monitoring UI problems described in #7054 (with screenshots). There is one commit for
each cause.
1.
fix: Keep the baseline out of the Monitoring pages' window views/monitoring/metrics/featuresreturns the baseline row alongside the windows, dated by the day itwas computed. The pages treated it as a window. A new
pages/monitoring/metricSelection.tsholdsthe shared selection logic:
latestWindowMetric(): the Monitoring tab now shows the newest window, not the baseline comparedwith itself. It shows the baseline on its own ("Baseline Distribution", no overlay) only when no
window exists yet.
windowGranularities(): the detail page's View options no longer include the raw "baseline" row(it keeps its "Baseline (all data)" option), and are ordered shortest window first, so the page
opens on Daily.
seriesForGranularity(): the detail page's Time-Series Analysis plots one granularity (the viewedone, or the shortest when the baseline is viewed) instead of every row on one line.
2.
fix: Draw the Monitoring tab's baseline overlay on the window's binsThe overlay drew baseline bin i behind window bin i, although every stored histogram has its own
edges, and scaled both by raw counts, although the baseline has many more rows. A ×100 unit change
drew the same bars as a clean day.
alignBaselineToBins()(new,components/histogramAlignment.ts) moves each baseline bin's countonto the window's bins by overlap and scales it to the window's row count. Values are treated as
uniform within a bin; a zero-width bin (the SQL backends' constant-column form) as a point. A
constant window takes the whole baseline bin that contains its value, so a 0/1 flag compares
against the baseline's share of zeros.
it throws
TypeError: undefined is not iterable.Design notes
this window's rows.
/monitoring/metrics/featuresand the baseline'smetric_dateare unchanged.Changing either would change the REST response or the stored data (
metric_dateis part ofFEATURE_METRICS_PK), which seems out of scope for a UI fix.If you'd prefer, the two commits split cleanly into two PRs (they share no files).
Relation to #7056. I reported #7054 with this fix ready, and #7056 has since been opened for it
too. Both take the same overall approach. In case it helps you choose or combine them, the
differences I see are:
is treated as a point, so the overlay still works when either side is constant. fix(ui): stop treating the monitoring baseline as the latest window #7056 skips
zero-width bins.
share outside the window's range (39% in the ×100 case in UI: Monitoring pages treat the baseline as a window, and its overlay uses mismatched bins #7054). fix(ui): stop treating the monitoring baseline as the latest window #7056 renormalizes the
overlapping part to 100%, so that share isn't visible.
Daily. fix(ui): stop treating the monitoring baseline as the latest window #7056 keeps the alphabetical order, so it opens on Biweekly.
I'm happy to go with whichever you prefer, or to fold anything useful from #7056 into this one.
Which issue(s) this PR fixes:
Fixes #7054
Checks
git commit -s)Testing Strategy
19 new jest tests:
metricSelection.test.ts(6): uses the rows/monitoring/metrics/featuresreturns after afeast monitor runon the demo. Covers the latest window, baseline only, empty windows,granularity order, and single-granularity series.
histogramAlignment.test.ts(10): edges, scaling, a shift past the baseline's range, zero-widthbins on either side, a constant 0/1 window, edge values, an empty baseline, a missing last edge.
HistogramChart.test.tsx(3, render): all three fail on master.Local results (Windows, Node 20;
.nvmrcpins 22, which CI uses):prettier --check,eslint,tscand thebuild:libtype check pass, and so does theproduction build with
CI=true.FeastUISansProviders.test.tsx, which fails 1–3of its tests per run on this machine: timeouts, and
getByRoleheading lookups right afternavigation. It fails the same way on unmodified master ("Feature Views" and "Data Sources" headings
not found).
Manual: the steps in #7054 on the
feast initdemo, served byfeast uiwith this build. Thescreenshots below cover the tab after a fresh run, on a clean day and on a ×100 day, plus the detail
page.
Misc
The overlay is approximate when values aren't uniform within a baseline bin. The stored histograms
keep only bin counts, so some within-bin assumption is unavoidable.
cc @jyejare




