[logging] Add Auto Refresh Dashboard event into dashboard logging #9087

graceguo-supercat · 2020-02-04T22:59:41Z

SUMMARY

In Dashboard, we allow dashboard owner set a refresh_frequency and save it with dashboard metadata. A dashboard with refresh_frequency (> 0) will auto force-refresh itself periodically once it is loaded in browser. But this auto-refresh event is not logged correctly in current dashboard logging mechanism.

This PR is to add auto refresh event, and a little code refactor for the period rendering function.

TEST PLAN

Test is covered by existed integration tests.

ADDITIONAL INFORMATION

REVIEWERS

@etr2460 @serenajiang

codecov-io · 2020-02-05T00:28:45Z

Codecov Report

Merging #9087 into master will decrease coverage by 0.43%.
The diff coverage is 34.48%.

@@            Coverage Diff            @@
##           master   #9087      +/-   ##
=========================================
- Coverage   59.44%     59%   -0.44%     
=========================================
  Files         369     371       +2     
  Lines       11749   11816      +67     
  Branches     2888    2902      +14     
=========================================
- Hits         6984    6972      -12     
- Misses       4586    4662      +76     
- Partials      179     182       +3

Impacted Files	Coverage Δ
...ssets/src/dashboard/containers/DashboardHeader.jsx	`100% <ø> (ø)`	⬆️
superset/assets/src/logger/LogUtils.js	`100% <ø> (ø)`	⬆️
...set/assets/src/dashboard/actions/dashboardState.js	`31.72% <0%> (-9.24%)`	⬇️
superset/assets/src/chart/chartAction.js	`43.24% <0%> (-4.71%)`	⬇️
.../src/dashboard/components/gridComponents/Chart.jsx	`67.41% <100%> (ø)`	⬆️
...uperset/assets/src/dashboard/components/Header.jsx	`41.79% <25%> (-0.28%)`	⬇️
...set/assets/src/dashboard/util/setPeriodicRunner.js	`66.66% <66.66%> (ø)`
...sets/src/explore/components/DisplayQueryButton.jsx	`51.38% <0%> (-1.74%)`	⬇️
...rset/assets/src/explore/reducers/exploreReducer.js	`25% <0%> (-0.93%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66fd177...bab5097. Read the comment docs.

1. add logging 2. refactor general periodical render function

etr2460

a couple questions

etr2460 · 2020-02-05T16:58:41Z

superset/assets/src/dashboard/components/Header.jsx

@@ -188,7 +188,7 @@ class Header extends React.PureComponent {

  forceRefresh() {
    if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);


what's the value in only passing around the chart keys instead of the chart object? It looks like we still need to get the whole object from state when we actually fetch the chart, so does this save much?

I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like added filter etc. User will expect periodically load the latest query params.

If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

etr2460 · 2020-02-05T17:00:10Z

superset/assets/src/dashboard/actions/dashboardState.js

-  };
-
-  return (dispatch, getState) => {
-    stopPeriodicRender();


did we lose the code where we cancel the periodic render function? i didn't see it anywhere here (but might've missed it)

graceguo-supercat · 2020-02-05T17:53:03Z

superset/assets/src/dashboard/util/setPeriodicRunner.js

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+const stopPeriodicRender = refreshTimer => {


Stop interval is here.

oh duh, sorry!

graceguo-supercat · 2020-02-05T17:57:35Z

superset/assets/src/dashboard/components/Header.jsx

@@ -188,7 +188,7 @@ class Header extends React.PureComponent {

  forceRefresh() {
    if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);


I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like added filter etc. User will expect periodically load the latest query params.

If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

etr2460

lgtm!

pull-request-size bot added the size/L label Feb 4, 2020

graceguo-supercat changed the title ~~[logging] Add Auto Refresh Dashboard user event into dashboard logging~~ [WIP][logging] Add Auto Refresh Dashboard user event into dashboard logging Feb 4, 2020

graceguo-supercat changed the title ~~[WIP][logging] Add Auto Refresh Dashboard user event into dashboard logging~~ [WIP][logging] Add Auto Refresh Dashboard event into dashboard logging Feb 4, 2020

graceguo-supercat force-pushed the gg-AddLoggingForAutoRefreshDashEvent branch from 9da4707 to b3a0d7b Compare February 5, 2020 00:03

graceguo-supercat changed the title ~~[WIP][logging] Add Auto Refresh Dashboard event into dashboard logging~~ [logging] Add Auto Refresh Dashboard event into dashboard logging Feb 5, 2020

[fix] Add Auto Refresh Dashboard user event into dashboard logging

bab5097

1. add logging 2. refactor general periodical render function

graceguo-supercat force-pushed the gg-AddLoggingForAutoRefreshDashEvent branch from b3a0d7b to bab5097 Compare February 5, 2020 08:21

etr2460 reviewed Feb 5, 2020

View reviewed changes

graceguo-supercat commented Feb 5, 2020

View reviewed changes

etr2460 approved these changes Feb 5, 2020

View reviewed changes

graceguo-supercat merged commit 26def81 into apache:master Feb 5, 2020

graceguo-supercat deleted the gg-AddLoggingForAutoRefreshDashEvent branch February 10, 2020 04:17

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[logging] Add Auto Refresh Dashboard event into dashboard logging #9087

[logging] Add Auto Refresh Dashboard event into dashboard logging #9087

graceguo-supercat commented Feb 4, 2020 •

edited

codecov-io commented Feb 5, 2020 •

edited

etr2460 left a comment

etr2460 Feb 5, 2020

graceguo-supercat Feb 5, 2020 •

edited

etr2460 Feb 5, 2020

graceguo-supercat Feb 5, 2020

etr2460 Feb 5, 2020

graceguo-supercat Feb 5, 2020 •

edited

etr2460 left a comment

[logging] Add Auto Refresh Dashboard event into dashboard logging #9087

[logging] Add Auto Refresh Dashboard event into dashboard logging #9087

Conversation

graceguo-supercat commented Feb 4, 2020 • edited

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented Feb 5, 2020 • edited

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Feb 5, 2020

Choose a reason for hiding this comment

graceguo-supercat Feb 5, 2020 • edited

Choose a reason for hiding this comment

etr2460 Feb 5, 2020

Choose a reason for hiding this comment

graceguo-supercat Feb 5, 2020

Choose a reason for hiding this comment

etr2460 Feb 5, 2020

Choose a reason for hiding this comment

graceguo-supercat Feb 5, 2020 • edited

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Feb 4, 2020 •

edited

codecov-io commented Feb 5, 2020 •

edited

graceguo-supercat Feb 5, 2020 •

edited

graceguo-supercat Feb 5, 2020 •

edited