feat(gaq): async chart data opt-in per request (async_mode) + auto-enable GTF - #43429
feat(gaq): async chart data opt-in per request (async_mode) + auto-enable GTF#43429villebro wants to merge 5 commits into
Conversation
Async chart-data execution is now opt-in per request: - /chart/data honors a request-level async_mode flag; an absent flag runs synchronously (HTTP 200), so programmatic API clients are unaffected. async still requires GLOBAL_ASYNC_QUERIES + full JSON result + caching on. - schema: document async_mode on ChartDataQueryContextSchema (popped in post_load — it's a request flag, not part of the QueryContext). - GLOBAL_ASYNC_QUERIES force-enables GLOBAL_TASK_FRAMEWORK at startup (async runs on GTF), with a log line; a custom feature-flag func can still override. - new config GLOBAL_ASYNC_QUERIES_DEFAULT (default True) — a frontend-only policy input for the UI's default async_mode, exposed via bootstrap conf. - update integration async tests to send async_mode; add a _should_run_async unit test.
Add resolveAsyncMode() policy chain — GLOBAL_ASYNC_QUERIES feature-flag gate → per-dashboard override → GLOBAL_ASYNC_QUERIES_DEFAULT — and inject async_mode into full-JSON chart-data renders (v1ChartDataRequest), so the UI stays async by default when the flag is on (exports and non-full result types stay sync). Thread the per-dashboard override (json_metadata.async_mode) from dashboard state through exploreJSON's requestParams. Adds resolveAsyncMode unit tests.
Add an 'Asynchronous query execution' control to the dashboard Properties modal (Deployment default / Force enabled / Force disabled), shown only when GLOBAL_ASYNC_QUERIES is enabled and persisted to json_metadata.async_mode. The value flows through exploreJSON → requestParams → resolveAsyncMode so the dashboard's charts honor the override; 'Deployment default' clears the key. A value set directly in the Advanced JSON editor takes precedence (mirrors refresh_frequency).
dosubot
Bot
added
api:charts
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/review |
Code Review Agent Run #17b7e5Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // Persist the per-dashboard async override (unless set directly in the | ||
| // Advanced JSON editor). 'default' clears it so the deployment default applies. | ||
| if (jsonMetadataObj.async_mode === undefined) { | ||
| if (asyncMode === 'default') { | ||
| delete jsonMetadataObj.async_mode; | ||
| } else { | ||
| jsonMetadataObj.async_mode = asyncMode; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The Advanced JSON editor is initialized from metadata that still contains an existing async_mode, so jsonMetadataObj.async_mode is already defined whenever a dashboard has a prior override. The conditional therefore skips the selector value entirely, preventing users from changing or clearing an existing force_on or force_off setting. Exclude this field from the editor copy or otherwise distinguish an intentional editor edit from the existing value before applying the selector. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Dashboard selector cannot change existing async overrides.
- ❌ Dashboard selector cannot restore deployment-default behavior.
- ⚠️ Users can still edit `async_mode` manually in Advanced JSON.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 360:368
**Comment:**
*Incorrect Condition Logic: The Advanced JSON editor is initialized from metadata that still contains an existing `async_mode`, so `jsonMetadataObj.async_mode` is already defined whenever a dashboard has a prior override. The conditional therefore skips the selector value entirely, preventing users from changing or clearing an existing `force_on` or `force_off` setting. Exclude this field from the editor copy or otherwise distinguish an intentional editor edit from the existing value before applying the selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. In To resolve this, you should remove the Would you like me to implement this fix and check for other comments on this PR? superset-frontend/src/dashboard/components/PropertiesModal/index.tsx |
| const asyncMode = | ||
| resultFormat === 'json' && | ||
| resultType === 'full' && | ||
| resolveAsyncMode(requestParams.async_mode_override); | ||
| const body = JSON.stringify( | ||
| asyncMode ? { ...payload, async_mode: true } : payload, | ||
| ); |
There was a problem hiding this comment.
Suggestion: Applying resolveAsyncMode inside the shared v1ChartDataRequest opts every default JSON/full caller into asynchronous execution when the feature flag and deployment default are enabled. Callers such as AlertReportModal call getChartDataRequest directly and then immediately read response.json.result without handling HTTP 202 task responses, so they receive {task_ids} and fail instead of receiving chart data. Restrict this opt-in to callers that implement the async polling path, or update every affected caller to await the async response. [api mismatch]
Severity Level: Major ⚠️
- ❌ Alert dashboard-filter values fail when async execution is enabled.
- ❌ Direct chart-data consumers receive task IDs instead of query results.
- ⚠️ Only deployments with async queries and uncached requests are affected.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.ts
**Line:** 454:460
**Comment:**
*Api Mismatch: Applying `resolveAsyncMode` inside the shared `v1ChartDataRequest` opts every default JSON/full caller into asynchronous execution when the feature flag and deployment default are enabled. Callers such as `AlertReportModal` call `getChartDataRequest` directly and then immediately read `response.json.result` without handling HTTP 202 task responses, so they receive `{task_ids}` and fail instead of receiving chart data. Restrict this opt-in to callers that implement the async polling path, or update every affected caller to await the async response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43429 +/- ##
==============================================
- Coverage 78.83% 78.82% -0.01%
==============================================
Files 2879 2881 +2
Lines 164332 164477 +145
Branches 37984 38024 +40
==============================================
+ Hits 129551 129656 +105
- Misses 32341 32381 +40
Partials 2440 2440
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…sistence Address review feedback on apache#43429: - async opt-in was applied inside the shared v1ChartDataRequest, so direct callers that read response.json.result (AlertReportModal, ViewQueryModal, fetchTopNValues, useResultsPane) would receive a 202 {task_ids} and break. Gate injection on an explicit getChartDataRequest({enableAsyncMode}) that only the 202-handling callers set (exploreJSON, FilterValue, FiltersConfigForm, DrillByModal, ChartVersionPreview). - the dashboard properties async override was never applied when a dashboard already had json_metadata.async_mode: the editor copy carried it so the '=== undefined' guard skipped the dropdown. Omit async_mode from the Advanced JSON editor copy (dropdown is the sole source of truth, mirroring show_chart_timestamps) and apply the dropdown value unconditionally on save.
| force: fromCache ? false : shouldRefresh, | ||
| ownState: filterOwnState, | ||
| // 202 is handled below via waitForAsyncData. | ||
| enableAsyncMode: true, |
There was a problem hiding this comment.
A dashboard set to force async off still enables this request whenever the deployment default is on, because this path never receives the dashboard override that exploreJSON passes to resolveAsyncMode. That makes filter values use a 202/polling flow while the dashboard's charts stay synchronous. Could this request receive the same override (and be covered for both override directions)?
| resultType === 'full' && | ||
| resolveAsyncMode(requestParams.async_mode_override); | ||
| const body = JSON.stringify( | ||
| asyncMode ? { ...payload, async_mode: true } : payload, |
There was a problem hiding this comment.
This makes async opt-in only for the v1ChartDataRequest producer. StatefulChart still posts full JSON chart-data requests directly, even though it already handles 202 responses, so its Matrixify cells now always run synchronously when this endpoint requires async_mode. Could the resolved policy be shared with that producer as well?
2 participants
SUMMARY
Step 5 of the GAQ→GTF epic (#43407): make async chart data opt-in per request. After step 4 moved async execution onto the Global Task Framework, this makes async opt-in per request and adds the deployment/per-dashboard controls for it.
Backend
async_modeper-request flag on/chart/data. Async runs only when the request setsasync_mode(an absent flag is treated as synchronous), so programmatic API clients keep the synchronous HTTP 200 flow. Async still additionally requiresGLOBAL_ASYNC_QUERIES, a full JSON result, and caching on. Documented onChartDataQueryContextSchema(popped inpost_load— it's a request flag, not part of theQueryContext). The decision is centralized inChartDataRestApi._should_run_async.GLOBAL_ASYNC_QUERIESforce-enablesGLOBAL_TASK_FRAMEWORKat startup (with a log line; a custom feature-flag func can still override), since async chart data runs on GTF.GLOBAL_ASYNC_QUERIES_DEFAULT(defaultTrue) — a frontend-only policy input for the UI's defaultasync_mode, exposed via bootstrap conf.Frontend
resolveAsyncMode()policy chain —GLOBAL_ASYNC_QUERIESfeature-flag gate → per-dashboard override →GLOBAL_ASYNC_QUERIES_DEFAULT. Full-JSON chart-data renders injectasync_modeaccordingly (exports and non-full result types stay synchronous), so the UI stays async by default when the flag is on.GLOBAL_ASYNC_QUERIESis enabled, persisted tojson_metadata.async_mode, and threaded throughexploreJSONso that dashboard's charts honor it. A value set directly in the Advanced JSON editor takes precedence (mirrorsrefresh_frequency).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no change to how charts render for end users. Adds an opt-in control in dashboard properties (visible only when
GLOBAL_ASYNC_QUERIESis enabled).TESTING INSTRUCTIONS
GLOBAL_ASYNC_QUERIESenabled, confirmGLOBAL_TASK_FRAMEWORKis enabled automatically (startup log line) and dashboards/explore load charts async by default.GLOBAL_ASYNC_QUERIES_DEFAULT = False; confirm the UI loads synchronously by default, and a dashboard set to "Force enabled" loads its charts async./api/v1/chart/datawithoutasync_mode→ HTTP 200 (synchronous); with"async_mode": true(flag on, full JSON, caching on) → HTTP 202 withtask_ids.Automated:
pytest tests/unit_tests/charts/test_chart_data_api.py tests/unit_tests/feature_flag_test.py tests/unit_tests/initialization_test.py; frontendnpm run test -- asyncMode chartActions PropertiesModal.ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES(force-enablesGLOBAL_TASK_FRAMEWORK)async_moderequest flag;GLOBAL_ASYNC_QUERIES_DEFAULTconfig)Targets the
gaq-to-gtffeature branch (part of #43407), notmaster.