-
Notifications
You must be signed in to change notification settings - Fork 33
New metric boxes #1279
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
base: main
Are you sure you want to change the base?
New metric boxes #1279
Conversation
I will fix the failing test in review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome!
Added my review comments. Currently experiencing an error. Otherwise looks good
Also one more request: It would be nice if the color line at the bottom of the boxes of the boxes changes when tabbing power -> energy -> co2. |
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request introduces a new design for the metric boxes on the frontend, making them more dynamic and organized with tabs for Power, Energy, and CO2. The backend is updated to calculate network power.
🔍 General Feedback
- The new metric box implementation in
frontend/js/helpers/metric-boxes.js
is a great improvement in terms of modularity and maintainability. - There is a critical bug in the new implementation where the metrics are not updated correctly for each phase.
- There are some minor issues with naming conventions and code cleanliness that should be addressed.
@gemini-cli /review |
@davidkopp @ribalba This should now trigger gemini PR reviews. Please try it out next time you make a PR. It should also work automatically on new PRs ... untested though. Currently Google seems overloaded ... so double check the actions if nothing happens (Failed example due to overload: https://github.com/green-coding-solutions/green-metrics-tool/actions/runs/16824036563) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request introduces a new design for the metric boxes in the frontend, moving from a static layout to a more dynamic and organized tabbed view for power, energy, and CO2 metrics. The changes also include updates to the backend to calculate and provide the necessary data for these new metrics.
🔍 General Feedback
- The new metric boxes are a great improvement, providing a much cleaner and more organized way to display the data.
- The use of WebComponents is a good choice for creating reusable UI elements.
- The code is generally well-written and easy to follow.
- There are a few instances where
let
is used whenconst
would be more appropriate. - Some functions can be simplified by using a single return statement.
|
||
const machine_power_metric_condition = (metric) => { | ||
if(metric.match(/^.*_power_.*_machine$/) !== null) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low: It's better to use a single return statement.
const machine_power_metric_condition = (metric) => { | |
if(metric.match(/^.*_power_.*_machine$/) !== null) return true; | |
return metric.match(/^.*_power_.*_machine$/) !== null; |
Speaking of overhead: The current Google recommendation implemntation does the following:
Edit: my analysis was wrong. The skipping code is actually used by the orchestrator before the VM gets spun up. Phew ... :) Less work for me Still: Will keep the solution in for a couple of days. Then alter it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request introduces a new design for the metric boxes in the frontend, using tabs to separate Power, Energy, and CO2 metrics. The backend is updated to calculate network power. The new UI is a good improvement, but there are a few minor issues to address.
🔍 General Feedback
- The new tabbed interface for metrics is a great enhancement for organizing and visualizing the data.
- The code is well-structured, but there are some leftover artifacts from the refactoring, like commented-out code and console logs, that should be cleaned up.
- There's a small regression in the UI where the metric type information (e.g.,
AVG + STD.DEV
) is no longer displayed.
@ArneTR put in all the stuff we talked about. The only thing I didn't come up with a nice way of doing is changing the name to "GPU Power" if there is no power reading for GPU. If this is important let me know and I will write a little for loop that does this at the end. Seemed "unclean" though. Also I couldn't test with RAM, GPU and Disk readings. Maybe we should add this to the example data? |
* main: Bump actions/create-github-app-token in /.github/workflows (#1284) Bump pylint from 3.3.7 to 3.3.8 (#1285) Updated echarts vor v.6.0 (#1282) Bump python from 3.13.5-slim-bookworm to 3.13.6-slim-bookworm in /docker (#1286) Bump deepdiff from 8.5.0 to 8.6.0 (#1283) Log parsing on full data (#1276) Bump redis from 6.3.0 to 6.4.0 (#1281) Added Gemini PR Review measurement_flow_process_duration is set in any case. Supplying a timeout of None will lead to no timeout Detect systemd cgroup path using cgroup name (instead of container id) (#1270) Bump redis from 6.2.0 to 6.3.0 (#1275) Removing sampling_rate for XGboost also from codespaces lsmod now uses sort Added kernel modules to listing (fix): Warnings error should not show on HTTP 204 - case is expected Fix NetworkIoCgroupSystemProvider doesn't create a value column (#1274) Clarifying comment for design decision Document the usage of cgroup system metric providers (#1273) Suspend check (#1269)
I merged in the branch and applied the suggestion to rename network_io_power_w => network_io_power_in_mW. What is also left unresolved, but was maybe a particular decision, is that the information of "Diff in %" and "AVG+STD.DEV" is not displayed anymore. I think it is quite clear what the values mean ... but open for discussion on this. What also is gone is the info from which source the metric originates. I personally always liked to quickly tell if it is an MCP value or an XGBoost value. Have you, @ribalba / @davidkopp ever used it though? |
Reply to @davidkopp comment:
|
This is the first version that is not totally complete. Needs testing on machines with more metrics providers enabled. Also I am sure @ArneTR will want some changes :)