chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata #13306
Conversation
…into hook-dashboard
…into hook-dashboard
pkdotson
force-pushed
the
hook-dashboard
branch
from
317ff12 to
e9c0a74
Compare
…into hook-dashboard
There was a problem hiding this comment.
Ok... went through all the code. All of it LGTM, though Python ain't my strong suit, so others may want to weigh in more there. I think the caching that @ktmud mentions will need a revisit, but everything here looks good to go to these eyes. Tested on the ephemeral environment, and I've failed to break it! Thanks for all the hard work here!
|
Ephemeral environment shutdown and build artifacts deleted. |
* master: fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) # Conflicts: # superset-frontend/src/dashboard/actions/hydrate.js # superset/views/core.py
* master: (53 commits) test: Adds tests to the UndoRedoKeyListeners component (#13919) chore: Adds dataMask reducer to reducerIndex (#13951) test: Tests audit for the Dashboard FilterBar (#13916) fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) Use consistent chart value (#14031) fix: Use superset generic db to catch external_metadata queries (#13974) ...
junlincc
added
risk:refactor
|
Unfortunately this PR caused dashboard performance issue for airbnb: Please see this chart, which is daily median value of all dashboard metadata load time (in airbnb). After metadata is loaded, dashboard will fetch charts and render charts. You will see that after 4/15 (release date) the dashboard metadata load time is increased a lot (about ~30%). In airbnb we have a lot of big dashboard, which have >100 charts from multiple datasets. The original solution, load full dashboard data in one API and cache the dashboard request, was one of proven way which improved dashboard load performance. But this good practice seems will not be carried over. Given the impact to our performance metric, airbnb is reverted this PR from our own production environment. I hope open source community and Preset team could re-consider this solution. By now we would suggest:
|
|
Thanks for reporting, Grace. Could you confirm the perf change is caused by this PR solely? SPA solution is designed to improve the application performance overall and we are happy to optimize the solution so that it can work for your use cases with large amount of data. I'm not sure adding feature flag to it is the best way to go, as we have way too many feature flags nowadays which increase the difficulty in testing. We requested a sample dataset from Airbnb multiple times, so that we can properly test each PR to make sure they work as expected when the dataset is extremely large.. but we haven't heard back in that sense........... We acknowledge the potential inconvenience we might have caused, let's work together towards solution. :) |
|
edit: I did not realize that the revert was done in Airbnb's fork. Thank you for giving notice of this issue! It is definitely in the plan to to tackle the problem. 😅 I think a FF would add too much complexity in this case, and would have a high probability of breaking something, so I'd like to add caching as an optimization instead. @ktmud do you have any availability to work together on a caching solution for this problem? |
|
@junlincc and @suddjian Thanks for the replies. Revert right now only happens in airbnb production, and I do not mean to revert in open source code base, because i agree all points that Aaron mentioned, we also prefer to fix forward. Revert is short term solution for airbnb. I will keep posting our perf data once the new results ready. Since revert was happened 4/19, it will take us a couple of days to aggregate data (airbnb aggregate superset perf data by daily). For proposed solutions, I also prefer cache all API than adding feature flag. If there are any progress in this direction, i am happy to bring back this PR and test it again in airbnb. |
|
@suddjian I don't have time to actually work on this but happy to review PRs if needed. I think one simple fix is to still use |
|
Hey @junlincc and @suddjian We have 1 more data point: once we reverted this PR (this PR and #18184 are the only change), the perf goes back to normal. We didn't run full 24 hours since the revert was happened after UTC 0:00. Can this chart enough to show the perf impact? As you see, more recent PRs are based on this change, it will be very hard for us to sync weekly with master branch and keep this PR out. Could you help to make some simple fix so that we can move forward, like @ktmud suggested above? Thanks! |
|
Thanks @graceguo-supercat, the perf impact is understood. Fixing this is my current focus. I am going to try to add caching on the endpoint since I think that'd be cleaner, and if it works then I think |
|
Thanks @suddjian In the new SPA solution, there are 3 extra api calls (to fetch dash/charts/datasets details) that issued after initial page load, will this extra calls make the overall initial metadata loading + processing faster? this is my biggest concern. IF this is the major blocker, what else we can do to improve it? |
|
I actually think loading data after initial page load will enable significant performance wins, when the SPA project is complete. A single page app will not need to load the page at all, so comparatively making three API calls to fetch data for the page will be a small task. Plus, no blank screen will mean better perceived performance as well. In the meantime, I think caching should solve your performance issues for the most part. |
…ad of bootstrapdata (apache#13306)" This reverts commit 4bb29b6.
…otstrapdata (apache#13306) * add hook for future async api calls * test to see conflict * add async middleware and update reducers * working async dashboard load * implement getcharts api * add user permissions to explore and dashboard bootstrap data * integrate api calls with getinitial state * update namings * accept an id or a slug in the dashboard charts api * add permissions function * fix merge * update state * get dashboard charts by id or slug * fix undefined states * variable names * stop using some more bootstrap data * fix metadata reference * remove unused bootstrap from the template * add errorboundry to dashboard * refactoring, fixing * update permissions * add just roles * id is supposed to be a string * unused vars * get datasources from api * make onError optional * use resource hooks, better error boundary * add loading state for dashboardroute * remove console * add conditional * more conditionals * testing out a possible fix for cypress * convert edit/standalone test to cypress * remove bootstrappy assertions * lint * fix dashboard edit history issue * rename stuff * address recent native filters schema change * remove unused getInitialState * remove .only from test * hooksy redux usage * Revert "more conditionals" This reverts commit 25c8ed6. * cleanup * undo unnecessary change * actually need conditions here * certainty * Revert "certainty" This reverts commit 77dea19. * more permutations (untested yolo) * Update superset-frontend/src/chart/chartReducer.ts Co-authored-by: Evan Rusackas <[email protected]> * import style * comment * cleaner dashboardInfo * remove debug code * use memo for getPermissions * fix lint * adjust name/location of DashboardPage * move logic for REMOVE_SLICE_LEVEL_LABEL_COLORS to DAO * stop using full_data() * remove unused (and now useless) json=true query param Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: Evan Rusackas <[email protected]>
…otstrapdata (apache#13306) * add hook for future async api calls * test to see conflict * add async middleware and update reducers * working async dashboard load * implement getcharts api * add user permissions to explore and dashboard bootstrap data * integrate api calls with getinitial state * update namings * accept an id or a slug in the dashboard charts api * add permissions function * fix merge * update state * get dashboard charts by id or slug * fix undefined states * variable names * stop using some more bootstrap data * fix metadata reference * remove unused bootstrap from the template * add errorboundry to dashboard * refactoring, fixing * update permissions * add just roles * id is supposed to be a string * unused vars * get datasources from api * make onError optional * use resource hooks, better error boundary * add loading state for dashboardroute * remove console * add conditional * more conditionals * testing out a possible fix for cypress * convert edit/standalone test to cypress * remove bootstrappy assertions * lint * fix dashboard edit history issue * rename stuff * address recent native filters schema change * remove unused getInitialState * remove .only from test * hooksy redux usage * Revert "more conditionals" This reverts commit 25c8ed6. * cleanup * undo unnecessary change * actually need conditions here * certainty * Revert "certainty" This reverts commit 77dea19. * more permutations (untested yolo) * Update superset-frontend/src/chart/chartReducer.ts Co-authored-by: Evan Rusackas <[email protected]> * import style * comment * cleaner dashboardInfo * remove debug code * use memo for getPermissions * fix lint * adjust name/location of DashboardPage * move logic for REMOVE_SLICE_LEVEL_LABEL_COLORS to DAO * stop using full_data() * remove unused (and now useless) json=true query param Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: Evan Rusackas <[email protected]>
mistercrunch
added
🏷️ bot
SUMMARY
This PR will begins the first steps of moving dashboard into spa while using the new dashboard api's
dashboard/:id/chartsand/dashboards/:id.Thanks for @suddjian for the pairing! :)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
updates to existing tests and end tests as need.
ADDITIONAL INFORMATION