Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dashboard: when updating parameters, run only relevant queries #3804

Merged
merged 4 commits into from
Aug 30, 2019
Merged

Dashboard: when updating parameters, run only relevant queries #3804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f729257
refresh only affected queries in dashboard when parameters are changed
Aug 20, 2019
ca66101
rename pendingParameters to updatedParameters
Aug 21, 2019
283498a
select which widgets to update according to their mapping as a dashbo…
Aug 27, 2019
9a9e4cb
use lodash's include
Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion client/app/components/Parameters.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,9 @@ export class Parameters extends React.Component {
applyChanges = () => {
const { onValuesChange, disableUrlUpdate } = this.props;
this.setState(({ parameters }) => {
const parametersWithPendingValues = parameters.filter(p => p.hasPendingValue);
forEach(parameters, p => p.applyPendingValue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can use parametersWithPendingValues here too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. Are you referring to using parametersWithPendingValues in the following line? As in

forEach(parametersWithPendingValues, p => p.applyPendingValue());

If that's what you mean, I deliberately avoided that in order to future-proof someone removing the hasPendingValue check in applyPendingValue without knowing of this external concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be that concerned about it (doesn't seem a likely thing or something that would cause a big issue here). But not a big deal in any case :)

onValuesChange();
onValuesChange(parametersWithPendingValues);
if (!disableUrlUpdate) {
updateUrl(parameters);
}
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/dashboards/widget.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
</div>
</div>
<div class="m-b-10" ng-if="$ctrl.localParametersDefs().length > 0">
<parameters parameters="$ctrl.localParametersDefs()" on-values-change="$ctrl.refresh"></parameters>
<parameters parameters="$ctrl.localParametersDefs()" on-values-change="$ctrl.forceRefresh"></parameters>
</div>
</div>

Expand Down
2 changes: 2 additions & 0 deletions client/app/components/dashboards/widget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ function DashboardWidgetCtrl($scope, $location, $uibModal, $window, $rootScope,
return this.widget.load(refresh, maxAge);
};

this.forceRefresh = () => this.load(true);

this.refresh = (buttonId) => {
this.refreshClickButtonId = buttonId;
this.load(true).finally(() => {
Expand Down
16 changes: 12 additions & 4 deletions client/app/pages/dashboards/dashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,16 @@ function DashboardCtrl(
this.extractGlobalParameters();
});

const collectFilters = (dashboard, forceRefresh) => {
const queryResultPromises = _.compact(this.dashboard.widgets.map((widget) => {
const collectFilters = (dashboard, forceRefresh, updatedParameters = []) => {
const affectedWidgets = updatedParameters.length > 0 ? this.dashboard.widgets.filter(
widget => Object.values(widget.getParameterMappings()).filter(
({ type }) => type === 'dashboard-level',
).some(
({ mapTo }) => _.includes(updatedParameters.map(p => p.name), mapTo),
),
) : this.dashboard.widgets;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncertain Array.includes is supported in Edge 11. Better use lodash.

Copy link
Contributor

@ranbena ranbena Aug 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering by name won't do as param names are unique only within a widget scope.
The same widget can show up twice in a dashboard - one with a dashboard-level param, the other with a widget-level param, and with this implementation, both will get refreshed instead of only one. (Same goes for different widgets that have the same param names).

Here's a demo https://deploy-preview-3804--redash-preview.netlify.com/dashboard/param-update-tester. Change the dashboard parameter, and all widgets refresh even though the middle one shouldn't, since it only has a widget-level param.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I switched it to use the proper parameter mapping.

Copy link
Contributor

@ranbena ranbena Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works perfectly 👍

I know you must have worked on making the filter logic readable but I think it's still a bit complex to comprehend. Here's my suggestion:

let affectedWidgets = dashboard.widgets;
if (updatedParameters.length) {
  const updateParamNames = updatedParameters.map(p => p.name); // do just once
  affectedWidgets = dashboard.widgets.filter(
    widget => _.chain(widget.getParameterMappings()) // I like chaining :P
      .filter({ type: 'dashboard-level' })
      .some(({ mapTo }) => _.includes(updateParamNames, mapTo))
      .value(),
  );
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chaining prevents webpack from including only the functions we use from lodash 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranbena thanks for introducing me to lodash chain sequences!

I don't think it would be significant to lose tree shaking on lodash, but I'm still not entirely convinced that the change from

filter({ type }) => type === 'dashboard-level')

to

filter({ type: 'dashboard-level' })

makes things simpler, especially given that some simpletons such as myself would possibly have to scratch their heads around _.chain and value(). The main disadvantage I see here is transforming this from an expression (const affectedWidgets = stuff) to a declarative block.

I personally find expressions a primary goal for achieving simplicity.

Copy link
Member

@gabrieldutra gabrieldutra Aug 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chaining prevents webpack from including only the functions we use from lodash

It does makes things a lot more readable, I wonder if it's really that costly when considering Lodash 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget it! Let's land this already!


const queryResultPromises = _.compact(affectedWidgets.map((widget) => {
widget.getParametersDefs(); // Force widget to read parameters values from URL
return widget.load(forceRefresh);
}));
Expand Down Expand Up @@ -202,9 +210,9 @@ function DashboardCtrl(

this.loadDashboard();

this.refreshDashboard = () => {
this.refreshDashboard = (parameters) => {
this.refreshInProgress = true;
collectFilters(this.dashboard, true).finally(() => {
collectFilters(this.dashboard, true, parameters).finally(() => {
this.refreshInProgress = false;
});
};
Expand Down