-
Notifications
You must be signed in to change notification settings - Fork 26
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
Grafana UI improvements #83
Conversation
8b89a00
to
7155d2b
Compare
7fa03ec
to
e24117c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Yun-Tang for the PR
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
build/charts/theia/provisioning/dashboards/flow_records_dashboard.json
Outdated
Show resolved
Hide resolved
@@ -1,647 +1,639 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the entire file has been changed due to some JSON re-formatting, which makes it hard to tell which functionality code has been changed.
Could you help verify only the following two changes mentioned by the PR description were made to this file?
- Remove mean value in throughput related table
- Add a threshold for time-series diagram to prevent points from connecting when the gap of time is > 90s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are 18 panels with these settings. So I removed 18 "means", "limit 50" and added 18 "spanNulls": 90000
"calcs": [ | ||
"mean" | ||
], | ||
"calcs": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we removed the "mean" metric from legend, we might need to regenerate new screenshots and replace those in the network-flowvisibility.md documentation. Could you send me the related new screenshots? I'll upload them to S3 bucket and give you back the links to put in the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, could you also attach some screenshots related to the UI changes in this PR? This will be very helpful for code review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated these screenshots in the description above. Please take a look to see if these photos are acceptable. Thanks.
} | ||
}, | ||
"format": 2 | ||
"queryType": "sql", | ||
"rawSql": "SELECT $__timeInterval(flowEndSeconds) as time, CONCAT(sourceNodeName, '->', destinationNodeName) as pair, SUM(throughput) as Node\nFROM flows_node_view\nWHERE sourceNodeName != '' AND destinationNodeName != ''\nAND sourcePodNamespace NOT IN ('kube-system', 'flow-visibility', 'flow-aggregator')\nAND destinationPodNamespace NOT IN ('kube-system', 'flow-visibility', 'flow-aggregator')\nAND $__timeFilter(time)\nGROUP BY time, pair\nORDER BY time\nLIMIT 50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To follow up on our previous discussion, do we need to remove the limit 50
from the time-series panels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we have to remove it. Otherwise, user might not be able to see the latest data.
Try to collect more feedback and suggestions, especially for the column name renaming. They are used in the table view of flow_records_dashboard(shown below). Yun-Tang also had a very good summarization list here: #76 (comment) Please help evaluate the column names from a user perspective. We are trying to make the names easier to understand for users who might not have enough pre-knowledge on Antrea specific terminologies. |
By the way, do we also want to include the star all the dashboards by default in this PR? |
8bca2e5
to
46a5a97
Compare
Codecov Report
@@ Coverage Diff @@
## main #83 +/- ##
=======================================
Coverage ? 24.72%
=======================================
Files ? 9
Lines ? 1278
Branches ? 0
=======================================
Hits ? 316
Misses ? 940
Partials ? 22
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
46a5a97
to
a1430a4
Compare
The new column names overall LGTM. Just have one suggestion for |
a1430a4
to
0a204b0
Compare
1. Map FlowEndReason value from int to string 2. Add user-friendly names for columns used in tables 3. Remove mean value in throughput related table 4. Add a threshold for time-series diagram to prevent points from connecting when the gap of time is > 90s Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
0a204b0
to
aa6b7bb
Compare
/theia-test-e2e |
LGTM |
Signed-off-by: Yun-Tang Hsu hsuy@vmware.com
Below are the updated screenshots related the changes in this PR:
![flow-visibility-flow-records-4](https://user-images.githubusercontent.com/59460118/182470094-506ad5d7-4b4b-4120-b9b7-0342d269282f.png)
2. pod-to-pod-dashboard:![flow-visibility-pod-to-pod-2](https://user-images.githubusercontent.com/59460118/182471348-646e1ae5-1f6e-48fa-b12c-ef2b453cc04d.png)
3. pod-to-service-dashboard:![flow-visibility-pod-to-service-2](https://user-images.githubusercontent.com/59460118/182471366-746c87a1-f283-4183-981a-5bad51a1a2e3.png)
4. pod-to-external-dashboard:![flow-visibility-pod-to-external-2](https://user-images.githubusercontent.com/59460118/182495184-b0ad1d45-3421-4824-be03-a256b097a80d.png)
5. node-to-node-dashboard:![flow-visibility-node-to-node-2](https://user-images.githubusercontent.com/59460118/182470336-bf028d6c-23c6-45fb-8fb0-53f0b1c253f2.png)
6. network-policy-dashboard: