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.

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

Add aggregate tables to speed up operational monitoring dashboards. #295

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

emtwo
Copy link
Contributor

@emtwo emtwo commented Nov 9, 2021

This PR adds aggregate tables to Operational Monitoring explores to speed up their loading. This branch in looker-hub has the changes that can be tested.

In order to add an aggregate table for each metric, the set of metrics associated with an explore need to be available to it including that metric's default values so it can be placed in the aggregate table. This data was already being processed in the dashboard.

In order to avoid doing the same processing twice - in explores and in dashboards, I factored out that processing in the dashboard and turned it into generic data functions that can be applied to any namespace, called only once to perform processing, and accessed in both explores and dashboards.

@emtwo emtwo force-pushed the emtwo/aggregate_tables branch 3 times, most recently from e569a2a to 59dbef1 Compare November 9, 2021 22:24
@emtwo emtwo marked this pull request as ready for review November 9, 2021 22:26
@emtwo emtwo requested a review from scholtzan November 9, 2021 22:26
Comment on lines +59 to +60
namespace_data = data.get("compute_opmon_dimensions", {}).get(
base_view_name, {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having to define data_functions and passing around namespace_data, I'm wondering if it is possible to move compute_opmon_dimensions() into the OperationalMonitoringExplore implementation. The explore does have access to all the view names and could pass them into compute_opmon_dimensions().
The compute_opmon_dimensions() is pretty specific to operational monitoring, so it's unlikely we will want to reuse it elsewhere. Moving it out of lookml_utils (which mostly contains generic lookml functions) might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The namespace_data gets passed into both the explore and the dashboard so I'm not sure OperationalMonitoringExplore is the right place for compute_opmon_dimensions().

I think there could be a separate utils file for opmon so that compute_opmon_dimensions() doesn't need to stay in lookmk_utils. Since the data produced by that function is used by both explores and dashboards, I think it doesn't necessarily belong in either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think this makes sense. I'm not super in love with having a new data_functions attribute, but it's okay for now.

As we are moving towards generating aggregate_tables for other generated explores, we might want to refactor this and have a better way of making dimensions available across views, explores and dashboards (+ some way to filter/transform them). I'll write up an issue to keep track of this.

For now moving compute_opmon_dimensions() into a separate file would be great.

Comment on lines +59 to +60
namespace_data = data.get("compute_opmon_dimensions", {}).get(
base_view_name, {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think this makes sense. I'm not super in love with having a new data_functions attribute, but it's okay for now.

As we are moving towards generating aggregate_tables for other generated explores, we might want to refactor this and have a better way of making dimensions available across views, explores and dashboards (+ some way to filter/transform them). I'll write up an issue to keep track of this.

For now moving compute_opmon_dimensions() into a separate file would be great.

@emtwo
Copy link
Contributor Author

emtwo commented Nov 10, 2021

As we are moving towards generating aggregate_tables for other generated explores, we might want to refactor this and
have a better way of making dimensions available across views, explores and dashboards (+ some way to filter/transform
them). I'll write up an issue to keep track of this.

I agree that I don't love the data_functions to be honest. I think it will be easier to design how this works once we do have another use-case of aggregate_tables

@emtwo emtwo force-pushed the emtwo/aggregate_tables branch from 59dbef1 to d765af0 Compare November 16, 2021 16:23
@emtwo emtwo merged commit a55120e into main Nov 16, 2021
@emtwo emtwo deleted the emtwo/aggregate_tables branch November 16, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants