fix(wasm): check if wasm enabled and no errors#1762
Conversation
L2D2Grafana
force-pushed
the
l2d2/wasm-fallback
branch
from
e6eee2f to
5d10717
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the WebAssembly initialization error handling by introducing explicit tracking of WASM initialization success. It replaces direct calls to wasmSupported() with a new isWasmInit() function that checks both WebAssembly support AND successful initialization of the augurs library. When WASM is unavailable or initialization fails, the UI automatically hides ML-based sorting options (changepoint and outliers) and falls back to standard deviation sorting.
Changes:
- Added
setWasmInit()andisWasmInit()functions to track WASM initialization state - Moved
SortBytype definition fromSortByScene.tsxtoservices/sorting.tsfor centralized management - Updated sorting UI to dynamically filter out WASM-dependent options when WASM is unavailable
- Added test coverage for WASM failure scenarios
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/sorting.ts | Added WASM initialization tracking with setWasmInit(), isWasmInit(), and getDefaultSortBy() functions; exported SortBy type and constants |
| src/module.tsx | Updated to call setWasmInit() with success/failure status after attempting to initialize WASM libraries |
| src/Components/ServiceScene/Breakdowns/SortByScene.tsx | Updated constructor to fallback to stdDev when stored preference requires WASM but WASM is unavailable; dynamically filters WASM-dependent options from UI |
| src/Components/ServiceScene/Breakdowns/SortByScene.test.tsx | Added test verifying WASM-dependent options are hidden when initialization fails |
| src/services/store.ts | Updated import to use SortBy type from services/sorting |
| src/services/fields.ts | Updated import to use SortBy type from services/sorting |
| src/Components/ServiceScene/Breakdowns/LabelValuesBreakdownScene.tsx | Updated to use getDefaultSortBy() and DEFAULT_SORT_DIRECTION constants |
| src/Components/ServiceScene/Breakdowns/LabelBreakdownScene.tsx | Updated to use getDefaultSortBy() and DEFAULT_SORT_DIRECTION constants |
| src/Components/ServiceScene/Breakdowns/FieldsBreakdownScene.tsx | Updated to use getDefaultSortBy() and DEFAULT_SORT_DIRECTION constants |
| src/Components/ServiceScene/Breakdowns/FieldValuesBreakdownScene.tsx | Updated to use getDefaultSortBy() and DEFAULT_SORT_DIRECTION constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (e) { | ||
| console.warn('grafana-lokiexplore-app: WebAssembly init failed, ML sorting disabled.', e); | ||
| setWasmInit(false); | ||
| } |
There was a problem hiding this comment.
When WASM is not supported (wasmSupported() returns false), the wasmInitSucceeded flag is never set to false. This means that if WASM is not supported, isWasmInit() will continue to return false (the default value), which is correct, but it would be clearer and more explicit to call setWasmInit(false) in an else block after the wasmSupported() check. This would make the intent clearer and ensure the flag is explicitly set in all code paths.
| } | |
| } | |
| } else { | |
| setWasmInit(false); |
There was a problem hiding this comment.
It is false by default.
nicwestvold
left a comment
nicwestvold
left a comment
There was a problem hiding this comment.
@L2D2Grafana, here is some (LLM) feedback. It seemed reasonable.
PR Review: l2d2/wasm-fallback
Overall: The approach is sound — the shift from wasmSupported() (browser check only) to isWasmInit() (captures actual initialization success) is the right fix. The fallback to stdDev, UI filtering of ML
options, and constructor preference override are all well-handled. A few issues worth addressing before merge.
Bugs / Correctness
[Medium] memoize cache is blind to WASM state — stale result risk
src/services/sorting.ts:75-91
The cache key for sortSeries includes sortBy and direction but not wasmInitSucceeded. If sortSeries(series, 'changepoint', 'desc') were ever called before WASM initialized, the try-catch fallback on
line 49 silently mutates sortBy to ReducerID.stdDev and the stdDev-sorted result gets cached under a key containing 'changepoint'. A later call with the same args after WASM succeeds returns the stale
result.
In the current flow this can't happen (WASM is fully initialized before the App component loads), but the invariant is unprotected. Recommend appending _${isWasmInit()} to the cache key on line 90.
[Low] isWasmInit() in Component render is not reactive
src/Components/ServiceScene/Breakdowns/SortByScene.tsx:113
const wasmInit = isWasmInit();
This reads a plain module-level boolean — not Grafana Scenes state — so no state change will trigger a re-render. Safe today because WASM is initialized exactly once before any scene renders, but the ordering
invariant is implicit. A comment documenting it would prevent a future regression if retry/re-init logic is ever added.
---
Missing Test Coverage
[Medium] No test for stored-preference override in constructor
src/Components/ServiceScene/Breakdowns/SortByScene.test.tsx
The most important regression path of this PR — "user has 'changepoint' stored in localStorage, WASM fails on load, constructor should fall back to stdDev" — has no test. Suggested case:
test('Falls back to stdDev when stored preference is changepoint but WASM init failed', () => {
setSortByPreference('fields', 'changepoint', 'desc');
setWasmInit(false);
scene = new SortByScene({ target: 'fields' });
expect(scene.state.sortBy).toBe(ReducerID.stdDev);
});
[Low] New sorting.ts exports have no unit tests
src/services/sorting.test.ts
setWasmInit, isWasmInit, and getDefaultSortBy are untested. At minimum getDefaultSortBy should be tested for both branches.
---
Nits
Duplicate test names — SortByScene.test.tsx:46,56
Both tests are named 'Reports criteria changes'. Line 56 tests direction change and should be renamed to 'Reports direction changes'.
Hardcoded 'changepoint' in test assertion — SortByScene.test.tsx:63
expect(eventSpy).toHaveBeenCalledWith(new SortCriteriaChanged('fields', 'changepoint', 'asc'), true);
Should use the DEFAULT_SORT_BY constant to stay in sync if the value ever changes.
setWasmInit(false) in catch is redundant — module.tsx:34
The flag defaults to false and is only set to true on success, so this call is a no-op. Harmless, but a comment explaining "explicit reset in case of partial initialization" would clarify intent.
defaultOptions re-allocated every render — SortByScene.tsx:114-121
When WASM is not initialized, .map(...).filter(...) creates new arrays on every render. Since isWasmInit() is stable after init, a useMemo([wasmInit]) would avoid the repeated allocation.
console.warn vs. logger — module.tsx:33
Rest of the codebase uses the logger service for errors. console.warn may be intentional here given the module entry-point bundle size constraint, but worth a comment.
---
What's Well Done
- Default wasmInitSucceeded = false means the fallback is automatic if setWasmInit(true) is never reached (e.g., unsupported browser) — no extra guard needed.
- All 4 breakdown scenes (FieldValues, Fields, Label, LabelValues) consistently updated — no missed call sites.
- Moving SortBy type and constants to services/sorting.ts removes the circular dependency risk from importing them via SortByScene.
- E2E selector fixes (getByLabel, modal-scoped getByRole) are strictly better and avoid false matches.
- The constructor finalSortBy guard correctly handles the persisted-preference-but-WASM-failed scenario at the point of object creation.
---
Summary
| Severity | File | Issue |
| Medium | sorting.ts:75-91 | memoize key missing WASM state |
| Medium | SortByScene.test.tsx | Missing test: stored preference override in constructor |
| Low | SortByScene.tsx:113 | isWasmInit() not reactive — document the ordering invariant |
| Low | sorting.test.ts | No unit tests for new exports |
| Nit | SortByScene.test.tsx:46,56 | Duplicate test names |
| Nit | SortByScene.test.tsx:63 | Hardcoded 'changepoint' instead of DEFAULT_SORT_BY |
| Nit | module.tsx:34 | Redundant setWasmInit(false) |
| Nit | SortByScene.tsx:114 | defaultOptions re-allocated each render |
| Nit | module.tsx:33 | console.warn vs. logger inconsistency |
Okay I updated per some of the comments, mostly the memoize one. It's not actually a problem since the app is only initialized once, but I suppose it helps with testing. |
|
@L2D2Grafana I'm ready to approve, but it looks like there is a test failure with 12.4.0. Here is what claude had to say: CI Failure Analysis Why only 12.4.0?
Root cause The PR changed two assertions in savedSearches.spec.ts: Before (both test assertions, lines ~104 and ~249): page.getByRole('button', { name: /Edit filter with key service_name/ })
.filter({ hasText: 'tempo-ingester' })After: page.getByLabel('Edit filter with key service_name')
// ...toContainText('tempo-ingester')The failing assertion receives In Grafana 12.4.0 the ad-hoc filter chip structure appears to have changed: The old selector was resilient to this because it combined role + regex name + Fix Revert the two filter chip assertions back to the original // tests/savedSearches.spec.ts ~line 104 await expect(
page.getByRole('button', { name: /Edit filter with key service_name/ })
.filter({ hasText: 'tempo-ingester' })
).toBeVisible();The |
Thanks, yeah the savedSearches e2e have been flaky and the compatibility was also failing! |
matyax
left a comment
matyax
left a comment
There was a problem hiding this comment.
Looking good! I'd appreciate renaming the initialization function because it's not properly reflecting the underlying situation.
src/module.tsx
Outdated
| if (wasmSupported()) { | ||
| try { | ||
| await Promise.all([initChangepoint(), initOutlier()]); | ||
| setWasmInit(true); |
There was a problem hiding this comment.
This is technically incorrect, because there's no WASM initialization. It's either supported or not, and what's initialized is Augurs.
| setWasmInit(true); | |
| setWasmSortInit(true); |
There was a problem hiding this comment.
Even better, since today we're using WASM but tomorrow it might be something else, and we don't need to update these functions just because the internal implementation changed.
| setWasmInit(true); | |
| setSortingInit(true); |
| }, | ||
| "dependencies": { | ||
| "@bsull/augurs": "^0.6.0", | ||
| "@bsull/augurs": "^0.10.2", |
There was a problem hiding this comment.
Awesome. No breaking changes?
| export const setWasmSortInit = (succeeded: boolean) => { | ||
| wasmInitSucceeded = succeeded; | ||
| }; | ||
|
|
||
| export const isWasmInit = () => wasmInitSucceeded; |
There was a problem hiding this comment.
| export const setWasmSortInit = (succeeded: boolean) => { | |
| wasmInitSucceeded = succeeded; | |
| }; | |
| export const isWasmInit = () => wasmInitSucceeded; | |
| export const setSortingInit = (succeeded: boolean) => { | |
| wasmInitSucceeded = succeeded; | |
| }; | |
| export const isSortingInit = () => wasmInitSucceeded; |
Worst case:
| export const setWasmSortInit = (succeeded: boolean) => { | |
| wasmInitSucceeded = succeeded; | |
| }; | |
| export const isWasmInit = () => wasmInitSucceeded; | |
| export const setWasmSortingInit = (succeeded: boolean) => { | |
| wasmInitSucceeded = succeeded; | |
| }; | |
| export const isWasmSortingInit = () => wasmInitSucceeded; |
Apologies, multitasking a lot.
Follow up to #1757 & https://raintank-corp.slack.com/archives/C075BDBTX96/p1771844319780149
This is a more thoughtful implementation for a fallback if there is an error with the augurs library. isWasmInit checks if webassembly is supported and if there was an error initialization grafana/augurs. Updated augurs to version 0.10.2 which should include the fallback when instantiateStreaming fails.
Testing