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

fix: topk suppport in 3 panels in snapmirror dashboard #1735

Merged
merged 7 commits into from
Feb 16, 2023

Conversation

Hardikl
Copy link
Contributor

@Hardikl Hardikl commented Feb 15, 2023

image

image

@cla-bot cla-bot bot added the cla-signed label Feb 15, 2023
@Hardikl Hardikl linked an issue Feb 15, 2023 that may be closed by this pull request
@@ -882,15 +882,15 @@
"pluginVersion": "8.1.8",
"targets": [
{
"expr": "avg (snapmirror_last_transfer_duration{source_cluster=~\"$SourceCluster\",source_volume=~\"$SourceVolume\",destination_volume=~\"$DestinationVolume\"}) by (relationship_id)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

avg by relationship_id doesn't do any work as relationship_id is unique

"format": "table",
"instant": true,
"interval": "",
"legendFormat": "",
"refId": "C"
},
{
"expr": "avg(snapmirror_last_transfer_size{source_cluster=~\"$SourceCluster\",source_volume=~\"$SourceVolume\",destination_volume=~\"$DestinationVolume\"}) by (relationship_id)",
"expr": "snapmirror_last_transfer_size{source_cluster=~\"$SourceCluster\",source_volume=~\"$SourceVolume\",destination_volume=~\"$DestinationVolume\",relationship_id=~\"$TopTransferDataById\"}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

it feels a bit surprising that the Last Transfers table is filtered by topk size. We should probably update the title of this panel to make that clear. Not sure folks will understand that the number of rows shown is a function of topk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to Top $TopResources Last Transfers by Transfer Data Size

Copy link
Contributor

Choose a reason for hiding this comment

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

may be give an all option in topk? Also this table should have filtering enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added filter for all columns.

For all option in topk,
Currently 500 is max value in topk. May be we provide highest value as some ~K, which gives all records.

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean enable All label in dropdown and give it a very high value?

Copy link
Contributor Author

@Hardikl Hardikl Feb 16, 2023

Choose a reason for hiding this comment

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

As topk(100, snapmirror_labels) function only allow number as arg not string, I would be thinking this way:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried this, But topk query would fail with this because not able to parse |.

image

image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Try setting a custom value for topk. Works fine for me.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed the custom all value....
made the changes for All.

@cgrinds
Copy link
Collaborator

cgrinds commented Feb 15, 2023

Not sure if this was introduced by your change, but looks like a column is missing?
image

@Hardikl
Copy link
Contributor Author

Hardikl commented Feb 16, 2023

Not sure if this was introduced by your change, but looks like a column is missing? image

it was there earlier, and not related to this table, so removed them.

@Hardikl
Copy link
Contributor Author

Hardikl commented Feb 16, 2023

image

@cgrinds cgrinds merged commit 2c10375 into release/23.02.0 Feb 16, 2023
@cgrinds cgrinds deleted the snapmirror_topok branch February 16, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional Panels for Snapmirror dashboard
3 participants