diff --git a/cmd/metrics/loader.go b/cmd/metrics/loader.go index 34874779..15be3a6d 100644 --- a/cmd/metrics/loader.go +++ b/cmd/metrics/loader.go @@ -57,7 +57,6 @@ type LoaderConfig struct { // Override configurations MetricDefinitionOverride string // For direct metric file override (legacy loader) EventDefinitionOverride string // For direct event file override (legacy loader) - ConfigFileOverride string // For config file that points to multiple files (perfmon loader) } type Loader interface { Load(config LoaderConfig) (metrics []MetricDefinition, groups []GroupDefinition, err error) @@ -80,7 +79,12 @@ type ComponentLoader struct { // NewLoader creates the right type of Loader for each CPU microarchitecture // Input is the CPU microarchitecture name as defined in the cpus module. -func NewLoader(uarch string) (Loader, error) { +// If useLegacyLoader is true, the legacy loader will be used regardless of microarchitecture. +func NewLoader(uarch string, useLegacyLoader bool) (Loader, error) { + if useLegacyLoader { + slog.Debug("Using legacy loader due to override", slog.String("uarch", uarch)) + return newLegacyLoader(), nil + } switch uarch { case cpus.UarchCLX, cpus.UarchSKX, cpus.UarchBDX, cpus.UarchBergamo, cpus.UarchGenoa, cpus.UarchTurinZen5, cpus.UarchTurinZen5c: slog.Debug("Using legacy loader for microarchitecture", slog.String("uarch", uarch)) diff --git a/cmd/metrics/loader_component.go b/cmd/metrics/loader_component.go index 7e191dd6..a53ad8ce 100644 --- a/cmd/metrics/loader_component.go +++ b/cmd/metrics/loader_component.go @@ -8,7 +8,6 @@ import ( "fmt" "io/fs" "log/slog" - "os" "path/filepath" "perfspect/internal/cpus" "perfspect/internal/util" @@ -20,7 +19,7 @@ import ( ) func (l *ComponentLoader) Load(loaderConfig LoaderConfig) ([]MetricDefinition, []GroupDefinition, error) { - metricDefinitions, err := l.loadMetricDefinitions(loaderConfig.MetricDefinitionOverride, loaderConfig.SelectedMetrics, loaderConfig.Metadata) + metricDefinitions, err := l.loadMetricDefinitions(loaderConfig.SelectedMetrics, loaderConfig.Metadata) if err != nil { return nil, nil, fmt.Errorf("failed to load metric definitions: %w", err) } @@ -59,29 +58,22 @@ type ComponentEvent struct { PublicDescription string `json:"PublicDescription"` } -func (l *ComponentLoader) loadMetricDefinitions(metricDefinitionOverridePath string, selectedMetrics []string, metadata Metadata) (metrics []MetricDefinition, err error) { - var bytes []byte - if metricDefinitionOverridePath != "" { - bytes, err = os.ReadFile(metricDefinitionOverridePath) // #nosec G304 - if err != nil { - return - } - } else { - var archDir string - archDir, err = getUarchDir(metadata.Microarchitecture) - if err != nil { - return nil, err - } - if bytes, err = resources.ReadFile(filepath.Join("resources", "component", archDir, "metrics.json")); err != nil { - return - } +func (l *ComponentLoader) loadMetricDefinitions(selectedMetrics []string, metadata Metadata) ([]MetricDefinition, error) { + archDir, err := getUarchDir(metadata.Microarchitecture) + if err != nil { + return nil, err + } + bytes, err := resources.ReadFile(filepath.Join("resources", "component", archDir, "metrics.json")) + if err != nil { + return nil, err } var componentMetricsInFile []ComponentMetric if err = json.Unmarshal(bytes, &componentMetricsInFile); err != nil { - return + return nil, err } evaluatorFunctions := getARMEvaluatorFunctions(metadata.ARMCPUID) + var metrics []MetricDefinition for i := range componentMetricsInFile { // a couple ARM metrics don't have MetricExpr, skip those // if selectedMetrics is empty, include all metrics diff --git a/cmd/metrics/loader_perfmon.go b/cmd/metrics/loader_perfmon.go index 7f135c72..f23c464e 100644 --- a/cmd/metrics/loader_perfmon.go +++ b/cmd/metrics/loader_perfmon.go @@ -189,22 +189,12 @@ func uarchToResourceName(uarch string) string { } func (l *PerfmonLoader) loadMetricsConfig(loaderConfig LoaderConfig) (MetricsConfig, error) { - var config MetricsConfig - var bytes []byte - if loaderConfig.ConfigFileOverride != "" { - var err error - bytes, err = os.ReadFile(loaderConfig.ConfigFileOverride) - if err != nil { - return MetricsConfig{}, fmt.Errorf("error reading metric config override file: %w", err) - } - } else { - var err error - resourceName := uarchToResourceName(loaderConfig.Metadata.Microarchitecture) - bytes, err = resources.ReadFile(filepath.Join("resources", "perfmon", resourceName, resourceName+".json")) - if err != nil { - return MetricsConfig{}, fmt.Errorf("error reading metrics config file: %w", err) - } + resourceName := uarchToResourceName(loaderConfig.Metadata.Microarchitecture) + bytes, err := resources.ReadFile(filepath.Join("resources", "perfmon", resourceName, resourceName+".json")) + if err != nil { + return MetricsConfig{}, fmt.Errorf("error reading metrics config file: %w", err) } + var config MetricsConfig if err := json.Unmarshal(bytes, &config); err != nil { return MetricsConfig{}, fmt.Errorf("error unmarshaling metrics config JSON: %w", err) } diff --git a/cmd/metrics/metrics.go b/cmd/metrics/metrics.go index da32ee8b..2c7bdf11 100644 --- a/cmd/metrics/metrics.go +++ b/cmd/metrics/metrics.go @@ -366,11 +366,11 @@ func getFlagGroups() []app.FlagGroup { }, { Name: flagEventFilePathName, - Help: "perf event definition file. Will override default event definitions. For legacy loader only.", + Help: "perf event definition file. Will override default event definitions. Must be used with --metricfile.", }, { Name: flagMetricFilePathName, - Help: "metric definition file. Will override default metric definitions.", + Help: "metric definition file. Will override default metric definitions. Must be used with --eventfile.", }, { Name: flagPerfPrintIntervalName, @@ -638,6 +638,9 @@ func validateFlags(cmd *cobra.Command, args []string) error { return workflow.FlagValidationError(cmd, fmt.Sprintf("failed to access metric file path: %s, error: %v", flagMetricFilePath, err)) } } + if (flagMetricFilePath != "" && flagEventFilePath == "") || (flagMetricFilePath == "" && flagEventFilePath != "") { + return workflow.FlagValidationError(cmd, "both --metricfile and --eventfile must be specified together") + } // input file path if flagInput != "" { if _, err := os.Stat(flagInput); err != nil { @@ -781,16 +784,9 @@ func getLoaderConfig(loader Loader, selectedMetrics []string, metadata Metadata, SelectedMetrics: selectedMetrics, Metadata: metadata, } - if _, ok := loader.(*PerfmonLoader); ok { - loaderConfig.ConfigFileOverride = metricsOverride - } else if _, ok := loader.(*LegacyLoader); ok { + if _, ok := loader.(*LegacyLoader); ok { loaderConfig.EventDefinitionOverride = eventsOverride loaderConfig.MetricDefinitionOverride = metricsOverride - } else if _, ok := loader.(*ComponentLoader); ok { - loaderConfig.MetricDefinitionOverride = metricsOverride - } else { - err := fmt.Errorf("unknown loader type: %T", loader) - panic(err) // this should never happen, but if it does, we want to know } return loaderConfig } @@ -802,7 +798,7 @@ func processRawData(localOutputDir string) error { } defer eventsFile.Close() // load metric and event group definitions - loader, err := NewLoader(metadata.Microarchitecture) + loader, err := NewLoader(metadata.Microarchitecture, flagMetricFilePath != "" && flagEventFilePath != "") if err != nil { err = fmt.Errorf("failed to create metric and event loader: %w", err) return err @@ -1242,7 +1238,7 @@ func prepareMetrics(targetContext *targetContext, localTempDir string, channelEr return } // load metric and event groups - loader, err := NewLoader(targetContext.metadata.Microarchitecture) + loader, err := NewLoader(targetContext.metadata.Microarchitecture, flagMetricFilePath != "" && flagEventFilePath != "") if err != nil { err = fmt.Errorf("failed to create metric and event loader: %w", err) _ = statusUpdate(myTarget.GetName(), fmt.Sprintf("Error: %s", err.Error())) diff --git a/cmd/metrics/trim.go b/cmd/metrics/trim.go index 6dff795e..1a0bf709 100644 --- a/cmd/metrics/trim.go +++ b/cmd/metrics/trim.go @@ -288,7 +288,7 @@ func getTrimmedSourceInfos(sourceDirOrFilename string) ([]trimSourceInfo, error) } func loadMetricDefinitions(metadata Metadata) ([]MetricDefinition, error) { - loader, err := NewLoader(metadata.Microarchitecture) + loader, err := NewLoader(metadata.Microarchitecture, false) if err != nil { return nil, fmt.Errorf("failed to create metric definition loader: %w", err) }