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

Hide zero, N/A graphs across runs #84

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Hide zero, N/A graphs across runs #84

merged 3 commits into from
Oct 10, 2023

Conversation

janaknat
Copy link
Contributor

@janaknat janaknat commented Oct 9, 2023

By default, don't show graphs for keys where the entries are all zero and N/A across all runs. There is a radio button on the top to see zero, N/A graphs if so needed.

Tabs updated: vmstat, diskstat, netstat, pmu stat, kernel config and sysctl.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

hidetest.tar.gz

@wash-amzn
Copy link
Contributor

What about the option for interrupts?

@janaknat
Copy link
Contributor Author

janaknat commented Oct 9, 2023

What about the option for interrupts?

We can't say if interrupt nr 1 for a system is the same as interrupt nr 1 for another system.

@@ -42,34 +42,54 @@
<div id="perfstat-runs"></div>
</div>
<div id="meminfo" class="tabcontent">
<div>
<h3>Hide zero, N/A graphs:</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some different wording like "Hide N/A and all-zeros graphs" would be better. "Hide empty graphs" would be on the other end where it's really simple at the cost of accuracy (somewhat).

Comment on lines 111 to 150
var elems = document.getElementsByClassName('vmstat-button');
for (var i=0; i < elems.length; i++) {
elems[i].addEventListener("click",function(evn: Event) {
if (this.id == "vmstat_hide_yes"){
vmStat(true);
} else {
vmStat(false);
}
})
}
var elems = document.getElementsByClassName('diskstat-button');
for (var i=0; i < elems.length; i++) {
elems[i].addEventListener("click",function(evn: Event) {
if (this.id == "diskstat_hide_yes"){
diskStats(true);
} else {
diskStats(false);
}
})
}
var elems = document.getElementsByClassName('meminfo-button');
for (var i=0; i < elems.length; i++) {
elems[i].addEventListener("click",function(evn: Event) {
if (this.id == "meminfo_hide_yes"){
meminfo(true);
} else {
meminfo(false);
}
})
}
var elems = document.getElementsByClassName('netstat-button');
for (var i=0; i < elems.length; i++) {
elems[i].addEventListener("click",function(evn: Event) {
if (this.id == "netstat_hide_yes"){
netStat(true);
} else {
netStat(false);
}
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's gotta be a way to turn this into a single loop over a set of tuples :p

It will be a lot less error-prone if something has to change later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I've refactored index.ts.

@@ -1,11 +1,11 @@
let got_kernel_config_data = false;
let current_diff_status = false;
let current_kernel_diff_status = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these changes? Certainly nothing to do with hiding zero graphs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Kernel Config and Sysctl used to default to 'No' for Diff. Based on that, the code was setup to calculate the keys for when the user presses 'Yes' for diff. We are now going to default to 'Yes' for Diff and the code now accounts for that.

@@ -5,78 +5,83 @@ var sysctl_runs: Map<string, RunEntry> = new Map<string, RunEntry>();
var sysctl_run_names: Array<string> = [];
var sysctl_common_keys: Array<string> = [];

function getSysctl(run, container_id, run_data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not related to hiding zero graphs

<input type="radio" class="sysctl-button" id="sysctl_diff_no" name="sysctlDiff">No</button>
</div>
<div id="sysctl-data-runs"></div>
</div>
<div id="vmstat" class="tabcontent">
<div>
<h3>Hide zero, N/A graphs:</h3>
<input type="radio" class="vmstat-button" id="vmstat_hide_yes" name="vmstatHide" checked>Yes</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

vmStatHide is not a very good name for what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'Hide zero' under 'vmstat' tab = 'vmStatHide'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Until you add the ability to hide something else and then have to rename this ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:O

Copy link
Contributor

@wash-amzn wash-amzn left a comment

Choose a reason for hiding this comment

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

The 'hide' naming is still not good

@janaknat
Copy link
Contributor Author

The 'hide' naming is still not good

Suggestions?

@wash-amzn
Copy link
Contributor

The 'hide' naming is still not good

Suggestions?

If you're going to have some sort of parameterization of the pages, then firstly it ideally wouldn't exist at the top-level index.ts. Failing that, you have to find some other naming that works for the two current use cases: diff vs non-diff (neither of which is hiding anything), and whether to hide all-zeros graphs (in which case hide is too vague and doomed to have to be renamed once we had the option to hide something else). Also, what's going to happen once a page has more that one boolean parameter (or non-boolean)?

@janaknat janaknat merged commit 0d07ed3 into main Oct 10, 2023
5 checks passed
@janaknat janaknat deleted the nozero branch November 7, 2023 00:12
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.

2 participants