Skip to content

Conversation

@janaknat
Copy link
Contributor

@janaknat janaknat commented Jul 1, 2024

This will allow the user to select the CPU(s) for all the graphs in CPU Utilization, Perf Stat and Interrupt tabs.

icocpus.tar.gz

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

@janaknat janaknat requested a review from a team as a code owner July 1, 2024 21:23
@wash-amzn
Copy link
Contributor

When "All" isn't selected, aggregate graphs (PMU Stats tab) should also be disabled.

y: y_data,
type: 'scatter',
};
if (interrupt_cpu_list.get(run).indexOf(cpu.toString()) == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Up in cpu_utilization you toString the cpu variable earlier.

If you're going to have repeated code like this (which ideally would be avoided), at least keep it consistent.

But really, try to find ways to extract out things like this which look identical, or almost identical, across multiple data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to the different way data is packaged. Also, we wanted to avoid checking for 'Aggregate' in multiple places.

grid-column: 1;
}

.configure {
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches a class, but you've named the button configure in index.html. That's why this background image isn't showing up. When you fix that, you'll see the issue with its position.

<body>
<div class="tab">
<div class="aperf">APerf</div>
<button class="tablinks" name="configure"><div class="icon-text">Configure</div><div class="configure"/></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to make this look different? Perhaps when the settings icon is working that will help, but I'm thinking of making it obviously not a peer to the other tabs which are data types.


info!("Creating APerf report...");
let ico = include_bytes!("html_files/favicon.ico");
let configure = include_bytes!("html_files/configure.png");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I see where it's used now. Multiple variables with similar names.

This will allow the user to select the CPU(s) for all the graphs in CPU
Utilization, Perf Stat and Interrupt tabs.
@janaknat janaknat merged commit 477d726 into main Jul 12, 2024
@janaknat janaknat deleted the cpus-configure branch July 12, 2024 16:23
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