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

Grafana UI improvements #83

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Jul 25, 2022

  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
  5. Remove limit 50 in time series panel (as it will only show the first 50 data point)

Signed-off-by: Yun-Tang Hsu hsuy@vmware.com

Below are the updated screenshots related the changes in this PR:

  1. flow-record-dashboard:

flow-visibility-flow-records-1

flow-visibility-flow-records-2

flow-visibility-flow-records-3

flow-visibility-flow-records-4

2. pod-to-pod-dashboard:

flow-visibility-pod-to-pod-2

3. pod-to-service-dashboard:

flow-visibility-pod-to-service-2

4. pod-to-external-dashboard:

flow-visibility-pod-to-external-1

flow-visibility-pod-to-external-2

5. node-to-node-dashboard:

flow-visibility-node-to-node-2

6. network-policy-dashboard:

flow-visibility-np-0

flow-visibility-np-1

flow-visibility-np-2

flow-visibility-np-3

@yuntanghsu yuntanghsu requested a review from heanlan July 25, 2022 22:58
@yuntanghsu yuntanghsu added this to the 0.2 milestone Jul 25, 2022
@yuntanghsu yuntanghsu added the enhancement New feature or request label Jul 25, 2022
@yuntanghsu yuntanghsu removed the enhancement New feature or request label Jul 25, 2022
@yuntanghsu yuntanghsu force-pushed the grafana_ui_improvement branch 2 times, most recently from 7fa03ec to e24117c Compare July 26, 2022 00:25
Copy link
Contributor

@heanlan heanlan left a 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

@@ -1,647 +1,639 @@
{
Copy link
Contributor

@heanlan heanlan Jul 27, 2022

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?

  1. Remove mean value in throughput related table
  2. Add a threshold for time-series diagram to prevent points from connecting when the gap of time is > 90s

Copy link
Contributor Author

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": [],
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@heanlan
Copy link
Contributor

heanlan commented Jul 27, 2022

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.

@yanjunz97 @dreamtalen @wsquan171 @salv-orlando @ziyouw

image

@heanlan
Copy link
Contributor

heanlan commented Jul 28, 2022

By the way, do we also want to include the star all the dashboards by default in this PR?

@yuntanghsu yuntanghsu force-pushed the grafana_ui_improvement branch 2 times, most recently from 8bca2e5 to 46a5a97 Compare July 29, 2022 22:22
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@4b15e06). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage        ?   24.72%           
=======================================
  Files           ?        9           
  Lines           ?     1278           
  Branches        ?        0           
=======================================
  Hits            ?      316           
  Misses          ?      940           
  Partials        ?       22           
Flag Coverage Δ
unit-tests 24.72% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out 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 4b15e06...a1430a4. Read the comment docs.

@dreamtalen
Copy link
Contributor

#76 (comment)

The new column names overall LGTM. Just have one suggestion for database insertion time, to keep consistent with other columns like start time of flow, how about changing it to insert time of flow or time of insertion of flow?

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>
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@dreamtalen
Copy link
Contributor

LGTM

@yuntanghsu yuntanghsu merged commit 3e15d4e into antrea-io:main Aug 4, 2022
@yuntanghsu yuntanghsu deleted the grafana_ui_improvement branch August 4, 2022 17:22
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.

User-friendly names for flow record fields in Grafana dashboard App Visibility Grafana dashboard enhancement
4 participants