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

Limit max data values of Grafana panels #3812

Merged
merged 1 commit into from May 27, 2022

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented May 19, 2022

Add limit to sql queries to set the max number of values displayed
on each panel.

  1. keep the charts readable when the input data is very large
  2. avoid taking too much time and resources to render the panels

@heanlan
Copy link
Contributor Author

heanlan commented May 19, 2022

This PR is brought by scale test. We expect the bug-fix PRs of scale test to be checked in in both Antrea and Theia repo.

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #3812 (7abb275) into main (040f07a) will decrease coverage by 18.32%.
The diff coverage is 37.70%.

❗ Current head 7abb275 differs from pull request most recent head 79aa792. Consider uploading reports for the commit 79aa792 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3812       +/-   ##
===========================================
- Coverage   64.38%   46.05%   -18.33%     
===========================================
  Files         280      247       -33     
  Lines       39754    35914     -3840     
===========================================
- Hits        25596    16541     -9055     
- Misses      12163    17738     +5575     
+ Partials     1995     1635      -360     
Flag Coverage Δ
e2e-tests 46.05% <37.70%> (?)
kind-e2e-tests ?
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go 16.90% <0.00%> (ø)
pkg/agent/multicast/mcast_controller.go 0.00% <0.00%> (-61.39%) ⬇️
pkg/agent/multicast/mcast_discovery.go 0.00% <0.00%> (-61.91%) ⬇️
pkg/agent/openflow/packetin.go 63.63% <ø> (ø)
pkg/agent/route/route_linux.go 27.92% <0.00%> (-21.88%) ⬇️
pkg/log/log_file.go 58.92% <ø> (-11.25%) ⬇️
pkg/log/log.go 83.33% <82.14%> (-16.67%) ⬇️
pkg/ipfix/ipfix_process.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ipfix/ipfix_registry.go 0.00% <0.00%> (-100.00%) ⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go 0.00% <0.00%> (-92.41%) ⬇️
... and 170 more

@heanlan heanlan added the area/flow-visibility Issues or PRs related to flow visibility support in Antrea label May 23, 2022
Comment on lines 948 to 953
- Also note that we set max number of values displayed on panels. For Sankey
diagrams, the limit is 50. For time-series line graphs, the limit is 100. For
pie charts, the limit is 25. The motivation is, when the input data is very
large, we want to keep the charts readable and avoid consuming too much time
and resources to render them.
Copy link

Choose a reason for hiding this comment

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

The changes look good to me. I only have a question about the thresholds. Did we have a few tests to choose proper values? For "proper", I mean it will be enough for most small/middle size deployments, so that most customers won't be bothered to change the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: I've changed the limit for line graphs from 100 to 50. As the line graphs and sankey diagrams are displaying the same group of objects. We'd better make them consistent.

Given the limit, to render the dashboard, Google chrome GPU takes ~300-400MB memory, renderer takes ~100-200MB memory, which I think is manageable for most of the case.

One of the major use case I can think of changing the limit is: when users add global filters to a dashboard. The filter will be added on top of the query results. That means, the query returns 50 values, while the filter can reduce the result to a smaller value less than 50. In that situation, users may want to increase the query limit, in order to show more values with the filter applied.

The reason for choosing 50 is because it is almost the upper bound to make the graph readable and displayed normally. When I created 60 connections. The left sankey diagram is with limit 60. The right one is with limit 50. We can tell with limit 60. The links can be failed to show.
image

Copy link

Choose a reason for hiding this comment

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

Thanks for the detailed information!

@heanlan heanlan force-pushed the limit-on-query branch 2 times, most recently from 01dd38c to b9f7d4f Compare May 24, 2022 19:50
ziyouw
ziyouw previously approved these changes May 26, 2022
salv-orlando
salv-orlando previously approved these changes May 26, 2022
Copy link
Contributor

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

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

LGTM

@heanlan heanlan requested a review from antoninbas May 26, 2022 14:32
ClickHouse SQL query for each individual panel.
ClickHouse and Grafana.

- Also note that we set max number of values displayed on panels. For Sankey
Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that we limit the number of values displayed on panels.

ClickHouse and Grafana.

- Also note that we set max number of values displayed on panels. For Sankey
diagrams, the limit is 50. For time-series line graphs, the limit is 50. For
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 a limit of 10,000 for some queries, but it's not mentioned here

If you want to stop filtering traffic by Namespace, or edit the panel limit,
you will need to edit the ClickHouse SQL query for each individual panel. Please
follow the [dashboards customization](#dashboards-customization) section for
more information. Especially, to edit the panel limit for pie charts, instead of
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Especially/As a special case

you will need to edit the ClickHouse SQL query for each individual panel. Please
follow the [dashboards customization](#dashboards-customization) section for
more information. Especially, to edit the panel limit for pie charts, instead of
editing query, please follow the [doc](https://grafana.com/docs/grafana/latest/visualizations/pie-chart-panel/#limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

editing the query

Add limit to sql queries to set the max number of values displayed
on each panel.
1. keep the charts readable when the input data is very large
2. avoid taking too much time and resources to render the panels

Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Copy link
Contributor Author

heanlan commented May 26, 2022

/test-all

@salv-orlando salv-orlando merged commit 220d6bf into antrea-io:main May 27, 2022
@heanlan heanlan deleted the limit-on-query branch May 31, 2022 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flow-visibility Issues or PRs related to flow visibility support in Antrea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants