Add brief system summary to metrics, telemetry, flame, and lock reports#304
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates energy performance bias (EPB) script definitions and updates report generation by adding a new brief system summary table that can be disabled with the --no-summary flag. Key changes include consolidating multiple EPB scripts into a single EpbScriptName, updating report table definitions and output functions to use the new summary table, and adding a new flag (--no-summary) in telemetry, lock, and flame commands to conditionally include the summary.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/script/script_defs.go | Consolidated EPB script definitions into a single script entry (EpbScriptName). |
| internal/report/table_helpers.go | Updated EPB output handling to use the consolidated EpbScriptName. |
| internal/report/table_defs.go | Introduced a new Brief System Summary table and updated existing references. |
| cmd/telemetry/telemetry.go | Added --no-summary flag to control inclusion of the brief system summary table. |
| cmd/lock/lock.go | Added --no-summary flag; potential duplicate addition of the summary table noted. |
| cmd/flame/flame.go | Added --no-summary flag to conditionally include the brief system summary. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new brief system summary report that can be optionally disabled with the --no-summary flag. Key changes include unifying the EPB script definitions, adding a new Brief System Summary table definition with associated processing logic, and updating the telemetry, metrics, lock, and flame commands to optionally include the system summary.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/script/script_defs.go | Unifies separate EPB scripts into a single EPB script definition. |
| internal/report/table_helpers.go | Updates frequency extraction and EPB parsing logic. |
| internal/report/table_defs.go | Adds the new Brief System Summary table definition and field mapping. |
| internal/common/common.go | Introduces accessor methods and refactors target output retrieval, including deferred temp directory removal. |
| cmd/telemetry/telemetry.go | Adds support for --no-summary flag to conditionally include system summary. |
| cmd/metrics/summary.go & metrics.go | Updates metadata processing to handle the new brief system summary. |
| cmd/metrics/metadata.go | Modifies metadata loading to conditionally retrieve system summary. |
| cmd/lock/lock.go, cmd/flame/flame.go | Updates flags and table selections to incorporate the brief system summary. |
Files not reviewed (1)
- cmd/metrics/resources/base.html: Language not supported
Comments suppressed due to low confidence (1)
internal/report/table_helpers.go:344
- The condition change from checking a higher index of specCoreFrequencies to always using index 1 may alter the intended behavior for selecting the max frequency. Verify that this new logic correctly matches the expected data structure.
if err == nil && len(specCoreFrequencies) > 1 && len(specCoreFrequencies[1]) > 1 {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… named variable for clarity
There was a problem hiding this comment.
Pull Request Overview
This PR implements a brief system summary for metrics, telemetry, flame, and lock reports and adds a new flag (--no-summary) to disable the summary. Key changes include refactoring and unifying EPB script definitions, updating table definitions to add a brief summary table, and adding the new flag to multiple CLI commands while updating related metadata collection and target clean‐up logic.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/script/script_defs.go | Consolidated EPB scripts by unifying OS and BIOS references into a single EPBScriptName. |
| internal/report/table_helpers.go | Adjusted the condition for selecting maximum core frequency from the specCoreFrequencies array. |
| internal/report/table_defs.go | Added a new brief system summary table definition and retrieval function. |
| internal/common/common.go | Updated RetrieveScriptOutputs and target clean-up logic with improved defer capture. |
| cmd/telemetry/telemetry.go | Introduced the --no-summary flag to exclude the brief system summary table from telemetry. |
| cmd/metrics/summary.go | Modified JSON metadata processing to purge certain fields and include system info. |
| cmd/metrics/metrics.go | Updated target clean-up and metrics preparation to capture defer variables correctly. |
| cmd/metrics/metadata.go | Enhanced LoadMetadata with concurrent collection of system summary and robust error handling. |
| cmd/lock/lock.go | Added conditional inclusion of the brief system summary table based on the new flag. |
| cmd/flame/flame.go | Updated flame command flag handling to conditionally include the brief system summary table. |
| cmd/config/config.go | Improved defer target capture to ensure proper clean-up when removing temporary directories. |
Files not reviewed (1)
- cmd/metrics/resources/base.html: Language not supported
Comments suppressed due to low confidence (7)
internal/script/script_defs.go:375
- The new naming for the unified EPB script consolidates previous OS and BIOS variants; please verify that all downstream references now correctly use EpbScriptName.
}, EpbScriptName: {
cmd/telemetry/telemetry.go:62
- [nitpick] The addition of the --no-summary flag is clear; please ensure its usage is consistently documented and applied across all related commands.
flagNoSystemSummary bool
cmd/metrics/metrics.go:727
- [nitpick] The use of a captured variable for defer is good practice; ensure this pattern is uniformly applied elsewhere to avoid closure capture issues.
deferTarget := myTarget // create a new variable to capture the current value
cmd/lock/lock.go:197
- [nitpick] The conditional addition of the brief system summary table based on the --no-summary flag is clear; verify that related documentation and tests reflect this new behavior.
if !flagNoSystemSummary {
cmd/flame/flame.go:177
- [nitpick] The implementation conditionally includes the brief system summary table when the --no-summary flag is not set; please confirm that the flag behavior meets user expectations.
if !flagNoSystemSummary {
cmd/config/config.go:411
- [nitpick] The defer capture pattern is correctly applied here; verify that similar defer usages throughout the code use this pattern for consistent behavior.
deferTarget := myTarget // create a new variable to capture the current value
internal/report/table_helpers.go:344
- The condition now assumes that specCoreFrequencies[1] holds the maximum frequency; please confirm that this logic aligns with all expected array structures in production.
if err == nil && len(specCoreFrequencies) > 1 && len(specCoreFrequencies[1]) > 1 {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
disable with --no-summary