← 输入框|github.com|刷新
Skip to content

fix: Keep the baseline out of monitoring windows and align its overlay - #7057

Open
Nitant28 wants to merge 2 commits into
feast-dev:masterfrom
Nitant28:fix/ui-monitoring-baseline-overlay
Open

Nitant28 wants to merge 2 commits into
feast-dev:masterfrom
Nitant28:fix/ui-monitoring-baseline-overlay

Conversation

@Nitant28

Copy link
Copy Markdown

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/features returns the baseline row alongside the windows, dated by the day it
was computed. The pages treated it as a window. A new pages/monitoring/metricSelection.ts holds
the shared selection logic:

  • latestWindowMetric(): the Monitoring tab now shows the newest window, not the baseline compared
    with 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 viewed
    one, 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 bins

The 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 count
    onto 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.
  • The legend states the share of the baseline outside the window's range.
  • A non-numeric baseline (left from before a feature changed type) is no longer overlaid. On master
    it throws TypeError: undefined is not iterable.

Design notes

  • Scaling instead of shares: scaling to the window's row count keeps the y-axis and tooltips in
    this window's rows.
  • Why UI-only: /monitoring/metrics/features and the baseline's metric_date are unchanged.
    Changing either would change the REST response or the stored data (metric_date is part of
    FEATURE_METRICS_PK), which seems out of scope for a UI fix.
  • Untouched: the categorical chart and the time-series component.

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:

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

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

19 new jest tests:

  • metricSelection.test.ts (6): uses the rows /monitoring/metrics/features returns after a
    feast monitor run on 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-width
    bins 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.
    • A baseline with 100× the rows draws window bars 1.6 px tall against 160 px baseline bars.
    • A window above the baseline's range still gets full-height baseline bars.
    • A categorical baseline throws.

Local results (Windows, Node 20; .nvmrc pins 22, which CI uses):

  • The new tests pass on this branch, rebased on master at e304a84.
  • prettier --check, eslint, tsc and the build:lib type check pass, and so does the
    production build with CI=true.
  • Full suite: 38 of 39 pass. The one failure is in FeastUISansProviders.test.tsx, which fails 1–3
    of its tests per run on this machine: timeouts, and getByRole heading lookups right after
    navigation. It fails the same way on unmodified master ("Feature Views" and "Data Sources" headings
    not found).

Manual: the steps in #7054 on the feast init demo, served by feast ui with this build. The
screenshots 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
after-1-fresh-run
after-2-clean-day
after-3-x100-unit-bug
after-4-detail-page-opens-on-daily
after-5-detail-time-series-daily-only

Signed-off-by: Nitant28 <singhnitant28@gmail.com>
Signed-off-by: Nitant28 <singhnitant28@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI: Monitoring pages treat the baseline as a window, and its overlay uses mismatched bins

1 participant