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

Jump to bottom

OM127 - 7.0 node view changes #76

Merged
merged 9 commits into from
Nov 7, 2023
Merged

OM127 - 7.0 node view changes #76

merged 9 commits into from
Nov 7, 2023

Conversation

mphanias
Copy link
Contributor

initial checkin with all 7.0 changes
7.0 based theme to combine 6.0 and 7.0 changes while showing state, topk and time-series values

@mphanias
OM127 - 7.0 node view changes
082b2c8 
initial checkin with all 7.0 changes
7.0 based theme to combine 6.0 and 7.0 changes while showing state, topk and time-series values
@mphanias mphanias requested review from sunilvirus and hev October 23, 2023 06:18
@hev
Copy link
Collaborator

hev commented Oct 24, 2023

Reviewed this across a split cluster. Planning some more testing, but some initial notes:

  • Having clear version is really helpful for supporting upgrade
Node_Overview_7_-_Dashboards_-_Grafana

Alerting feedback
View_panel_-_Node_Overview_-_Aerospike_-_Dashboards_-_Grafana

  • I'd expect this to be a sum of number of alerts on the node
  • I don't think we should include warnings in this view. Just Critical IMO

Best Practice Check
Node_Overview_7_-_Dashboards_-_Grafana_and_Multi_Cluster_View_-_Aerospike_-_Dashboards_-_Grafana

  • Should be yellow to match warnings color on alerts view

  • Ideally best practice checks would describe the specific best practice. Or we can add instructions to run the command asadm show best-practice

  • It may make sense to have an orange box for errors level alerts here also.

  • Finally I believe it is more correct to use Record Count instead of Object count.

hev
hev requested changes Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@hev hev left a comment

Choose a reason for hiding this comment

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

See previous comment.

@hev hev mentioned this pull request Oct 24, 2023
Merged
@mphanias
OM127 - review feedback
43bd9d5 
updated layout, alerts panel type
adjusted panel widths to fit layout
@mphanias
OM127 - review feedback
eb94e34 
1. corrected panel titles and data-types
2. corrected leged in time-series to show last, min, max, and mean
3. alerts critical color and url are corrected
@mphanias
OM127 - review feedback
54c4aa0 
corrected panel titles to include total/max
updated data-types
@mphanias
OM127 - review feedback
f3fb006 
1. corrected query labels
2. fixed table header to hide time-stamp and node-server-ip
3. fixed timeseries thresholds with rate-interval instead of 1m
@hev
Copy link
Collaborator

hev commented Oct 30, 2023

One more comment.

I wonder about offering a namepsace variable on this dashboard. Strictly speaking it seems like we should only filter on node for this dashbaord (I think its fine to keep it just wanted to note it here).

@mphanias
OM127 - review feedback
3950a64 
fixed bugs
- topk showing more values, fixed query type as instant
- alerts url bug, fixed url params are specific values not generic variables
@mphanias
Copy link
Contributor Author

mphanias commented Nov 1, 2023

  1. changed the alerts panel as a pie chart - with this we can show all alerts in relevant colour
  2. alerts panel is modded to show sum of all alerts
  3. best-practice and node-up panels now represent same colour as warn & critical like alerts
  4. added "asadm show best-practice" in the best-practice description
  5. namespace filter not-changed as we felt as it would be helpful
  6. "master objects" panel title is left as-is instead of records as metric name is master_objects

@mphanias mphanias requested a review from hev November 1, 2023 04:42
@mphanias
OM127 - review feedback
99402f6 
made required changes as per review comments
1. best-practice and node-up panel colors reflecting as per alert severity
2. added asadm best-practice command in description of panel
@mphanias
OM127 - unit tests
aeb6f43 
added new test-cases for connectors
fixed query extractor all dashboard queries in subfolder properly
re-generated base-line and mock results
@mphanias
OM127- corrected descriptions
8a6a470
hev
hev approved these changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@hev hev left a comment

Choose a reason for hiding this comment

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

Color changes look good.

@sunilvirus sunilvirus merged commit a0a81cd into dev Nov 7, 2023
1 check passed
@sunilvirus sunilvirus deleted the OM127 branch November 7, 2023 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hev hev hev approved these changes

@sunilvirus sunilvirus Awaiting requested review from sunilvirus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mphanias @hev @sunilvirus