From c33ecd286184be6b4cc172ff8b82597464ae2f0d Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 19:25:42 +0100 Subject: [PATCH 01/24] Create tool to automate reviews of metrics files This adds an automatic tool to review metrics files. It should run as part of PRB and post back the results of computing the diff. --- .github/workflows/metrics_analysis.yml | 158 +++++ .../yamltests/GitMetricsFileFinder.java | 177 +++++ .../yamltests/MetricsDiffAnalyzer.java | 635 ++++++++++++++++++ .../relational/yamltests/MetricsInfo.java | 84 +++ .../yamltests/MetricsStatistics.java | 179 +++++ .../yamltests/YamlExecutionContext.java | 195 ++++++ .../queryconfigs/CheckExplainConfig.java | 23 +- .../yamltests/MetricsDiffAnalyzerTest.java | 404 +++++++++++ .../yamltests/MetricsDiffIntegrationTest.java | 176 +++++ .../yamltests/MetricsStatisticsTest.java | 234 +++++++ .../metrics-diff-test-base.metrics.yaml | 34 + .../metrics-diff-test-head.metrics.yaml | 45 ++ .../metrics-outlier-test-base.metrics.yaml | 155 +++++ .../metrics-outlier-test-head.metrics.yaml | 155 +++++ .../metrics-diff/test-base.metrics.yaml | 47 ++ .../metrics-diff/test-head.metrics.yaml | 58 ++ .../test-new-queries.metrics.yaml | 12 + yaml-tests/yaml-tests.gradle | 34 + 18 files changed, 2785 insertions(+), 20 deletions(-) create mode 100644 .github/workflows/metrics_analysis.yml create mode 100644 yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java create mode 100644 yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java create mode 100644 yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java create mode 100644 yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java create mode 100644 yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java create mode 100644 yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java create mode 100644 yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsStatisticsTest.java create mode 100644 yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-base.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-head.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-base.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-head.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/test-base.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/test-head.metrics.yaml create mode 100644 yaml-tests/src/test/resources/metrics-diff/test-new-queries.metrics.yaml diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml new file mode 100644 index 0000000000..fa0f9a2b20 --- /dev/null +++ b/.github/workflows/metrics_analysis.yml @@ -0,0 +1,158 @@ +name: Metrics Diff Analysis + +on: + pull_request: + branches: [ main ] + paths: + - '**/*.metrics.yaml' + - '**/*.metrics.binpb' + - 'yaml-tests/**' + - '.github/workflows/metrics_analysis.yml' + +permissions: + contents: read + pull-requests: write + +jobs: + metrics-analysis: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + fetch-depth: 0 # Need full history for git diff + + - name: Setup Base Environment + uses: ./actions/setup-base-env + + - name: Run metrics diff analysis + id: metrics-diff + run: | + # Run the analysis and capture output + ./gradlew :yaml-tests:analyzeMetrics \ + -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ + -PmetricsAnalysis.repositoryRoot="$( pwd )" \ + -PmetricsAnalysis.output="$(pwd)/metrics-analysis-output.txt" \ + -PmetricsAnalysis.outlierQueries="$(pwd)/outlier-queries.txt" \ + || echo "ANALYSIS_FAILED=true" >> $GITHUB_OUTPUT + + # Read the output for PR comment + { + echo 'ANALYSIS_REPORT<> $GITHUB_OUTPUT + + if [[ -f "$(pwd)/outlier-queries.txt" ]] ; then + { + echo 'OUTLIERS_REPORT<> $GITHUB_OUTPUT + fi + + - name: Check for outliers + id: check-changes + run: | + if [[ -f outlier-queries.txt ]] ; then + echo "SIGNIFICANT_CHANGES=true" >> $GITHUB_OUTPUT + else + echo "SIGNIFICANT_CHANGES=false" >> $GITHUB_OUTPUT + fi + + - name: Comment on PR + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const output = `${{ steps.metrics-diff.outputs.OUTLIERS_REPORT }}`; + const hasSignificantChanges = '${{ steps.check-changes.outputs.SIGNIFICANT_CHANGES }}' === 'true'; + + const commentBody = `## 📊 Metrics Diff Analysis + + ${output} + +
+ ℹī¸ About this analysis + + This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into: + + - **New queries**: Queries added in this PR + - **Dropped queries**: Queries removed in this PR + - **Plan changed + metrics changed**: Expected when query plans change + - **Metrics only changed**: ⚠ī¸ Potential concern - same plan but different metrics + + The last category may indicate planner regressions and should be investigated. +
+ `; + + // Find existing comment + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + }); + + const existingComment = comments.find(comment => + comment.user.type === 'Bot' && comment.body.includes('📊 Metrics Diff Analysis') + ); + + if (existingComment) { + // Update existing comment + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existingComment.id, + body: commentBody + }); + } else { + // Create new comment + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: commentBody + }); + } + + - name: Add inline comments for outliers + uses: actions/github-script@v7 + if: steps.check-changes.outputs.SIGNIFICANT_CHANGES == 'true' + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const fs = require('fs'); + + // Parse the outliers report analysis output to find files with notable queries + const output = `${{ steps.metrics-diff.outputs.ANALYSIS_REPORT }}`; + + // Each query appears separated by a blank line + const queries = output.split('\n\n'); + + for (const query of queries) { + var newl = query.indexOf('\n'); + if (newl < 0) { + continue; + } + const info = query.substring(0, newl); + const match = info.match(/^(.+\.metrics\.yaml):(\d+): (.+)$/); + if (match) { + const [, filePath, lineNumber, query] = match; + const data = query.substring(newl, query.length); + changedFiles.push({ filePath, query }); + try { + await github.rest.pulls.createReviewComment({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + body: `**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n${data}\n` + path: filePath, + line: lineNumber, + side: 'RIGHT' + }); + } catch (error) { + console.log(`Could not add comment to ${filePath}: ${error.message}`); + } + } + } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java new file mode 100644 index 0000000000..d50bf394d3 --- /dev/null +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java @@ -0,0 +1,177 @@ +/* + * GitMetricsFileFinder.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import com.apple.foundationdb.record.logging.KeyValueLogMessage; +import com.apple.foundationdb.relational.api.exceptions.ErrorCode; +import com.apple.foundationdb.relational.api.exceptions.RelationalException; +import com.google.common.collect.ImmutableSet; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.annotation.Nonnull; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +/** + * Utility class for finding changed metrics files using git integration. + * This class provides methods to identify metrics files that have changed between different git references. + */ +public final class GitMetricsFileFinder { + private static final Logger logger = LoggerFactory.getLogger(GitMetricsFileFinder.class); + + private GitMetricsFileFinder() { + // Utility class + } + + /** + * Finds all metrics YAML files that have changed between two git references. + * + * @param baseRef the base git reference (e.g., "main", "HEAD~1", commit SHA) + * @param repositoryRoot the root directory of the git repository + * @return set of paths to changed metrics files (both .yaml and .binpb) + * @throws RelationalException if git command fails or repository is not valid + */ + @Nonnull + public static Set findChangedMetricsYamlFiles(@Nonnull final String baseRef, + @Nonnull final Path repositoryRoot) throws RelationalException { + final List changedFiles = getChangedFiles(baseRef, repositoryRoot); + final ImmutableSet.Builder metricsFiles = ImmutableSet.builder(); + + for (final var filePath : changedFiles) { + if (isMetricsYamlFile(filePath)) { + metricsFiles.add(repositoryRoot.resolve(filePath)); + } + } + + return metricsFiles.build(); + } + + /** + * Gets a list of all files changed between two git references. + * + * @param baseRef the base reference + * @param repositoryRoot the repository root directory + * @return list of relative file paths that have changed + * @throws RelationalException if git command fails + */ + @Nonnull + private static List getChangedFiles(@Nonnull final String baseRef, + @Nonnull final Path repositoryRoot) throws RelationalException { + final var command = List.of("git", "diff", "--name-only", baseRef); + try { + if (logger.isDebugEnabled()) { + logger.debug(KeyValueLogMessage.of("Executing git command", + "root", repositoryRoot, + "command", String.join(" ", command))); + } + final var processBuilder = new ProcessBuilder(command) + .directory(repositoryRoot.toFile()) + .redirectErrorStream(true); + + final var process = processBuilder.start(); + final var result = new ArrayList(); + + try (final var reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) { + String line; + while ((line = reader.readLine()) != null) { + result.add(line.trim()); + } + } + + final var exitCode = process.waitFor(); + if (exitCode != 0) { + throw new RelationalException( + "Git command failed with exit code " + exitCode + ": " + String.join(" ", command), + ErrorCode.INTERNAL_ERROR); + } + + return result; + } catch (final IOException | InterruptedException e) { + throw new RelationalException("Failed to execute git command: " + String.join(" ", command), ErrorCode.INTERNAL_ERROR, e); + } + } + + /** + * Checks if a file path represents a metrics file (either .yaml or .binpb). + * + * @param filePath the file path to check + * @return true if the file is a metrics file + */ + private static boolean isMetricsYamlFile(@Nonnull final String filePath) { + return filePath.endsWith(".metrics.yaml"); + } + + /** + * Checks if a file exists at the specified path. + * + * @param filePath the path to check + * @return true if the file exists + */ + public static boolean fileExists(@Nonnull final Path filePath) { + return Files.exists(filePath) && Files.isRegularFile(filePath); + } + + /** + * Gets the file path for a specific git reference (commit). + * This method checks out the file content at the specified reference. + * + * @param filePath the relative file path + * @param gitRef the git reference + * @param repositoryRoot the repository root + * @return path to a temporary file containing the content at the specified reference + * @throws RelationalException if git command fails + */ + @Nonnull + public static Path getFileAtReference(@Nonnull final String filePath, + @Nonnull final String gitRef, + @Nonnull final Path repositoryRoot) throws RelationalException { + try { + final var tempFile = Files.createTempFile("metrics-diff-", gitRef + ".metrics.yaml"); + final var command = List.of("git", "show", gitRef + ":" + filePath); + final var processBuilder = new ProcessBuilder(command) + .directory(repositoryRoot.toFile()) + .redirectOutput(tempFile.toFile()) + .redirectErrorStream(false); + + final var process = processBuilder.start(); + final var exitCode = process.waitFor(); + + if (exitCode != 0) { + Files.deleteIfExists(tempFile); + throw new RelationalException( + "Git show command failed with exit code " + exitCode + " for " + gitRef + ":" + filePath, + ErrorCode.INTERNAL_ERROR); + } + + return tempFile; + } catch (final IOException | InterruptedException e) { + throw new RelationalException( + "Failed to get file content at reference " + gitRef + ":" + filePath, ErrorCode.INTERNAL_ERROR, e); + } + } +} diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java new file mode 100644 index 0000000000..da32239b57 --- /dev/null +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -0,0 +1,635 @@ +/* + * MetricsDiffAnalyzer.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import com.apple.foundationdb.record.logging.KeyValueLogMessage; +import com.apple.foundationdb.relational.api.exceptions.ErrorCode; +import com.apple.foundationdb.relational.api.exceptions.RelationalException; +import com.apple.foundationdb.relational.util.Assert; +import com.apple.foundationdb.relational.yamltests.generated.stats.PlannerMetricsProto; +import com.beust.jcommander.JCommander; +import com.beust.jcommander.Parameter; +import com.beust.jcommander.ParameterException; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.protobuf.Descriptors; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.List; +import java.util.Map; + +/** + * Main analyzer for comparing metrics between different git references. + * This tool identifies queries that have been added, removed, or changed, and provides + * statistical analysis of metrics differences. + */ +public final class MetricsDiffAnalyzer { + private static final Logger logger = LogManager.getLogger(MetricsDiffAnalyzer.class); + + private final String baseRef; + private final Path repositoryRoot; + + /** + * Command line arguments for the metrics diff analyzer. + */ + public static class Arguments { + @Parameter(names = {"--base-ref", "-b"}, description = "Git reference for baseline (e.g., 'main', commit SHA)", order = 0) + public String baseRef = "main"; + + @Parameter(names = {"--repository-root", "-r"}, description = "Path to git repository root", order = 1) + public String repositoryRoot = "."; + + @Parameter(names = {"--output", "-o"}, description = "Output file path (writes to stdout if not specified)", order = 2) + public String outputPath; + + @Parameter(names = {"--outlier-queries"}, description = "Output file path to specifically list data for outlier queries", order = 3) + public String outlierQueryPath; + + @Parameter(names = {"--help", "-h"}, description = "Show help message", help = true, order = 4) + public boolean help; + } + + public MetricsDiffAnalyzer(@Nonnull final String baseRef, + @Nonnull final Path repositoryRoot) { + this.baseRef = baseRef; + this.repositoryRoot = repositoryRoot; + } + + /** + * Main entry point for command-line usage. + */ + public static void main(String[] args) { + final Arguments arguments = new Arguments(); + final JCommander commander = JCommander.newBuilder() + .addObject(arguments) + .programName("MetricsDiffAnalyzer") + .build(); + + try { + commander.parse(args); + } catch (ParameterException e) { + System.err.println("Error: " + e.getMessage()); + commander.usage(); + System.exit(1); + } + + if (arguments.help) { + commander.usage(); + return; + } + + final String baseRef = arguments.baseRef; + final Path repositoryRoot = Paths.get(arguments.repositoryRoot); + final Path outputPath = arguments.outputPath != null ? Paths.get(arguments.outputPath) : null; + + try { + final var analyzer = new MetricsDiffAnalyzer(baseRef, repositoryRoot); + final var analysis = analyzer.analyze(); + final var report = analysis.generateReport(); + + if (outputPath != null) { + Files.writeString(outputPath, report); + logger.info(KeyValueLogMessage.of( + "Wrote metrics diff report", + "output", outputPath)); + } else { + System.out.println(report); + } + + if (arguments.outlierQueryPath != null) { + String outlierReport = analysis.generateOutlierQueryReport(); + if (!outlierReport.isEmpty()) { + final Path outlierQueryPath = Paths.get(arguments.outlierQueryPath); + Files.writeString(outlierQueryPath, outlierReport); + logger.info(KeyValueLogMessage.of("Wrote outlier report", + "output", outlierQueryPath)); + } + } + } catch (final Exception e) { + logger.error("Failed to analyze metrics diff", e); + System.err.println("Error: " + e.getMessage()); + System.exit(1); + } + } + + /** + * Performs the metrics diff analysis. + * + * @return analysis results + * + * @throws RelationalException if analysis fails + */ + @Nonnull + public MetricsAnalysisResult analyze() throws RelationalException { + logger.info(KeyValueLogMessage.of("Starting metrics diff analysis", + "base", baseRef)); + + // Find all changed metrics files + final var changedFiles = GitMetricsFileFinder.findChangedMetricsYamlFiles(baseRef, repositoryRoot); + logger.info(KeyValueLogMessage.of("Found changed metrics files", + "base", baseRef, + "file_count", changedFiles.size())); + + final var analysisBuilder = MetricsAnalysisResult.newBuilder(repositoryRoot); + + for (final var filePath : changedFiles) { + analyzeYamlFile(filePath, analysisBuilder); + } + + return analysisBuilder.build(); + } + + /** + * Analyzes a single metrics file for changes. + */ + private void analyzeYamlFile(@Nonnull final Path yamlPath, + @Nonnull final MetricsAnalysisResult.Builder analysisBuilder) throws RelationalException { + if (logger.isDebugEnabled()) { + logger.debug(KeyValueLogMessage.of("Analyzing YAML metrics file", + "path", yamlPath)); + } + + // Make sure this is a YAML metrics file + if (!yamlPath.toString().endsWith(".metrics.yaml")) { + throw new RelationalException("Unexpected file type for file (expected .metrics.yaml): " + yamlPath, ErrorCode.INTERNAL_ERROR); + } + + // Load base (old) metrics + Map baseMetrics; + try { + final var baseYamlFile = GitMetricsFileFinder.getFileAtReference( + repositoryRoot.relativize(yamlPath).toString(), baseRef, repositoryRoot); + baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile(baseYamlFile); + Files.deleteIfExists(baseYamlFile); // Clean up temp file + } catch (final RelationalException e) { + // File might not exist in base ref (new file) + if (logger.isDebugEnabled()) { + logger.debug(KeyValueLogMessage.of("Could not load base metrics", + "path", yamlPath), e); + } + baseMetrics = ImmutableMap.of(); + } catch (IOException e) { + throw new RelationalException("unable to delete temporary file", ErrorCode.INTERNAL_ERROR, e); + } + + // Load head (new) metrics + final Map headMetrics; + if (GitMetricsFileFinder.fileExists(yamlPath)) { + headMetrics = YamlExecutionContext.loadMetricsFromYamlFile(yamlPath); + } else { + // File was deleted + headMetrics = ImmutableMap.of(); + } + + // Compare the metrics + compareMetrics(baseMetrics, headMetrics, yamlPath, analysisBuilder); + } + + /** + * Compares metrics between base and head versions. + */ + @VisibleForTesting + public void compareMetrics(@Nonnull final Map baseMetrics, + @Nonnull final Map headMetrics, + @Nonnull final Path filePath, + @Nonnull final MetricsAnalysisResult.Builder analysisBuilder) { + + final var baseIdentifiers = baseMetrics.keySet(); + final var headIdentifiers = headMetrics.keySet(); + + // Find new queries (in head but not in base) + int newCount = 0; + for (final var identifier : headIdentifiers) { + if (!baseIdentifiers.contains(identifier)) { + newCount++; + analysisBuilder.addNewQuery(filePath, identifier, headMetrics.get(identifier)); + } + } + + // Find dropped queries (in base but not in head) + int droppedCount = 0; + for (final var identifier : baseIdentifiers) { + if (!headIdentifiers.contains(identifier)) { + droppedCount++; + analysisBuilder.addDroppedQuery(filePath, identifier, baseMetrics.get(identifier)); + } + } + + // Find changed queries (in both base and head) + int changedCount = 0; + for (final var identifier : headIdentifiers) { + if (baseIdentifiers.contains(identifier)) { + final MetricsInfo baseInfo = baseMetrics.get(identifier); + final MetricsInfo headInfo = headMetrics.get(identifier); + + final var planChanged = !baseInfo.getExplain().equals(headInfo.getExplain()); + final var metricsChanged = YamlExecutionContext.areMetricsDifferent( + baseInfo.getCountersAndTimers(), headInfo.getCountersAndTimers()); + + if (planChanged && metricsChanged) { + changedCount++; + analysisBuilder.addPlanAndMetricsChanged(filePath, identifier, baseInfo, headInfo); + } else if (!planChanged && metricsChanged) { + changedCount++; + analysisBuilder.addMetricsOnlyChanged(filePath, identifier, baseInfo, headInfo); + } + // If plan changed but metrics didn't change, we don't report it. This could happen if + // there's some cosmetic change to the plan, like one operator's serialization is changed. + // It could also be something more interesting, but we're generally more concerned + // with changed metrics anyway + } + } + + if (logger.isDebugEnabled()) { + logger.debug(KeyValueLogMessage.of("Analyzed metrics file", + "path", filePath, + "new", newCount, + "dropped", droppedCount, + "changed", changedCount)); + } + } + + /** + * Results of metrics analysis containing all detected changes. + */ + public static class MetricsAnalysisResult { + private final List newQueries; + private final List droppedQueries; + private final List planAndMetricsChanged; + private final List metricsOnlyChanged; + @Nullable + private final Path repositoryRoot; + + private MetricsAnalysisResult(@Nonnull final List newQueries, + @Nonnull final List droppedQueries, + @Nonnull final List planAndMetricsChanged, + @Nonnull final List metricsOnlyChanged, + @Nullable final Path repositoryRoot) { + this.newQueries = newQueries; + this.droppedQueries = droppedQueries; + this.planAndMetricsChanged = planAndMetricsChanged; + this.metricsOnlyChanged = metricsOnlyChanged; + this.repositoryRoot = repositoryRoot; + } + + public List getNewQueries() { + return newQueries; + } + + public List getDroppedQueries() { + return droppedQueries; + } + + public List getPlanAndMetricsChanged() { + return planAndMetricsChanged; + } + + public List getMetricsOnlyChanged() { + return metricsOnlyChanged; + } + + public boolean hasSignificantChanges() { + return !droppedQueries.isEmpty() || !metricsOnlyChanged.isEmpty(); + } + + /** + * Helper method to format a query change for display with relative path and line number. + */ + @Nonnull + private String formatQueryDisplay(@Nonnull final QueryChange change) { + final var relativePath = repositoryRoot != null ? repositoryRoot.relativize(change.filePath) : change.filePath; + int lineNumber = -1; + if (change.newInfo != null) { + lineNumber = change.newInfo.getLineNumber(); + } else if (change.oldInfo != null) { + lineNumber = change.oldInfo.getLineNumber(); + } + final String lineInfo = lineNumber > 0 ? (":" + lineNumber) : ""; + return String.format("%s%s: `%s`", relativePath, lineInfo, change.identifier.getQuery()); + } + + @Nonnull + public String generateReport() { + final var report = new StringBuilder(); + report.append("# Metrics Diff Analysis Report\n\n"); + + report.append("## Summary\n\n"); + report.append(String.format("- New queries: %d\n", newQueries.size())); + report.append(String.format("- Dropped queries: %d\n", droppedQueries.size())); + report.append(String.format("- Plan changed + metrics changed: %d\n", planAndMetricsChanged.size())); + report.append(String.format("- Plan unchanged + metrics changed: %d\n", metricsOnlyChanged.size())); + report.append("\n"); + + if (!newQueries.isEmpty()) { + report.append("## New Queries\n\n"); + for (final var change : newQueries) { + report.append(String.format("- %s\n", formatQueryDisplay(change))); + } + report.append("\n"); + } + + if (!droppedQueries.isEmpty()) { + report.append("## Dropped Queries\n\n"); + for (final var change : droppedQueries) { + report.append(String.format("- %s\n", formatQueryDisplay(change))); + } + report.append("\n"); + } + + appendChangesList(report, planAndMetricsChanged, "Plan and Metrics Changed"); + appendChangesList(report, metricsOnlyChanged, "Only Metrics Changed"); + + return report.toString(); + } + + public String generateOutlierQueryReport() { + final List outliers = findAllOutliers(); + if (outliers.isEmpty()) { + return ""; + } + + final StringBuilder report = new StringBuilder(); + for (QueryChange queryChange : outliers) { + report.append(formatQueryDisplay(queryChange)).append("\n"); + appendMetricsDiff(report, queryChange); + report.append("\n"); + } + return report.toString(); + } + + private void appendStatisticalSummary(@Nonnull final StringBuilder report, @Nonnull final MetricsStatistics stats) { + for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + final var fieldStats = stats.getFieldStatistics(fieldName); + final var absoluteFieldStats = stats.getAbsoluteFieldStatistics(fieldName); + if (fieldStats.hasChanges() || absoluteFieldStats.hasChanges()) { + report.append(String.format("**`%s`**:\n", fieldName)); + report.append(String.format(" - Average change: %.1f\n", fieldStats.getMean())); + report.append(String.format(" - Average absolute change: %.1f\n", absoluteFieldStats.getMean())); + report.append(String.format(" - Median change: %d\n", fieldStats.getMedian())); + report.append(String.format(" - Median absolute change: %d\n", absoluteFieldStats.getMedian())); + report.append(String.format(" - Standard deviation: %.1f\n", fieldStats.getStandardDeviation())); + report.append(String.format(" - Standard absolute deviation: %.1f\n", absoluteFieldStats.getStandardDeviation())); + report.append(String.format(" - Range: %d to %d\n", fieldStats.getMin(), fieldStats.getMax())); + report.append(String.format(" - Range of absolute values: %d to %d\n", absoluteFieldStats.getMin(), absoluteFieldStats.getMax())); + report.append(String.format(" - Queries affected: %d\n\n", fieldStats.getChangedCount())); + } + } + } + + private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull List changes, String title) { + if (changes.isEmpty()) { + return; + } + report.append("## ").append(title).append("\n\n"); + report.append("Total: ").append(changes.size()).append(" quer").append(changes.size() == 1 ? "y" : "ies").append("\n\n"); + + // Statistical analysis + final var summary = calculateMetricsStatistics(changes); + report.append("### Statistical Summary (").append(title).append(")\n\n"); + appendStatisticalSummary(report, summary); + + // Show outliers for metrics-only changes (these are more concerning) + final var metricsOutliers = findOutliers(changes, summary); + report.append("### Significant Changes (").append(title).append(")\n\n"); + if (metricsOutliers.isEmpty()) { + report.append("No outliers detected.\n\n"); + } else { + for (final var change : metricsOutliers) { + report.append(String.format("- %s", formatQueryDisplay(change))); + if (change.oldInfo != null && change.newInfo != null) { + report.append("
\n"); + appendMetricsDiff(report, change); + } else { + report.append("\n"); + } + } + report.append("\n"); + } + + if (changes.size() > metricsOutliers.size()) { + int minorChanges = changes.size() - metricsOutliers.size(); + report.append("### Minor Changes (").append(title).append(")\n\nIn addition, there ") + .append(minorChanges == 1 ? "was " : "were ") + .append(minorChanges) + .append(" quer").append(minorChanges == 1 ? "y" : "ies") + .append(" with minor changes.\n\n"); + } + } + + private MetricsStatistics calculateMetricsStatistics(@Nonnull final List changes) { + final var statisticsBuilder = new MetricsStatistics.Builder(); + + for (final var change : changes) { + if (change.oldInfo != null && change.newInfo != null) { + final var oldMetrics = change.oldInfo.getCountersAndTimers(); + final var newMetrics = change.newInfo.getCountersAndTimers(); + final var descriptor = oldMetrics.getDescriptorForType(); + + for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + final var field = descriptor.findFieldByName(fieldName); + final var oldValue = (long) oldMetrics.getField(field); + final var newValue = (long) newMetrics.getField(field); + + if (oldValue != newValue) { + final var difference = newValue - oldValue; + statisticsBuilder.addDifference(fieldName, difference); + } + } + } + } + + return statisticsBuilder.build(); + } + + private List findAllOutliers() { + return ImmutableList.builder() + .addAll(findOutliers(planAndMetricsChanged)) + .addAll(findOutliers(metricsOnlyChanged)) + .build(); + } + + private List findOutliers(@Nonnull final List changes) { + final MetricsStatistics summary = calculateMetricsStatistics(changes); + return findOutliers(changes, summary); + } + + private List findOutliers(@Nonnull final List changes, @Nonnull final MetricsStatistics stats) { + if (changes.size() < 3) { + // Not enough data for meaningful outlier detection + return changes; + } + + final ImmutableList.Builder outliers = ImmutableList.builder(); + + for (final var change : changes) { + if (change.oldInfo != null && change.newInfo != null) { + final var oldMetrics = change.oldInfo.getCountersAndTimers(); + final var newMetrics = change.newInfo.getCountersAndTimers(); + final var descriptor = oldMetrics.getDescriptorForType(); + + boolean isOutlier = false; + for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + final var field = descriptor.findFieldByName(fieldName); + final var oldValue = (long) oldMetrics.getField(field); + final var newValue = (long) newMetrics.getField(field); + + if (oldValue != newValue) { + final var difference = newValue - oldValue; + if (isOutlierValue(stats.getFieldStatistics(fieldName), difference) + || isOutlierValue(stats.getAbsoluteFieldStatistics(fieldName), Math.abs(difference))) { + isOutlier = true; + break; + } + } + } + + if (isOutlier) { + outliers.add(change); + } + } + } + + return outliers.build(); + } + + private boolean isOutlierValue(MetricsStatistics.FieldStatistics fieldStats, long difference) { + // Consider it an outlier if it's more than 2 standard deviations from the mean + // or if it's a very large absolute change + Assert.thatUnchecked(fieldStats.hasChanges(), "Field stats should have at least one difference"); + final var zScore = Math.abs((difference - fieldStats.mean) / fieldStats.standardDeviation); + final var isLargeAbsoluteChange = Math.abs(difference) > Math.max(100, Math.abs(fieldStats.mean) * 2); + return zScore > 2.0 || isLargeAbsoluteChange; + } + + private void appendMetricsDiff(@Nonnull final StringBuilder report, + @Nonnull final QueryChange queryChange) { + Assert.thatUnchecked(queryChange.oldInfo != null, "old info must be set to display metrics diff"); + Assert.thatUnchecked(queryChange.newInfo != null, "new info must be set to display metrics diff"); + + final PlannerMetricsProto.CountersAndTimers oldMetrics = queryChange.oldInfo.getCountersAndTimers(); + final PlannerMetricsProto.CountersAndTimers newMetrics = queryChange.newInfo.getCountersAndTimers(); + + final var descriptor = oldMetrics.getDescriptorForType(); + + for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + final Descriptors.FieldDescriptor field = descriptor.findFieldByName(fieldName); + final long oldValue = (long)oldMetrics.getField(field); + final long newValue = (long)newMetrics.getField(field); + if (oldValue != newValue) { + final long change = newValue - oldValue; + final String changeStr = change > 0 ? "+" + change : String.valueOf(change); + report.append(String.format("    `%s`: %d -> %d (%s)
\n", fieldName, oldValue, newValue, changeStr)); + } + } + } + + public static MetricsAnalysisResult.Builder newBuilder() { + return new Builder(null); + } + + public static MetricsAnalysisResult.Builder newBuilder(@Nullable final Path repositoryRoot) { + return new Builder(repositoryRoot); + } + + public static class Builder { + private final ImmutableList.Builder newQueries = ImmutableList.builder(); + private final ImmutableList.Builder droppedQueries = ImmutableList.builder(); + private final ImmutableList.Builder planAndMetricsChanged = ImmutableList.builder(); + private final ImmutableList.Builder metricsOnlyChanged = ImmutableList.builder(); + @Nullable + private final Path repositoryRoot; + + public Builder(@Nullable final Path repositoryRoot) { + this.repositoryRoot = repositoryRoot; + } + + public Builder addNewQuery(@Nonnull final Path filePath, + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo info) { + newQueries.add(new QueryChange(filePath, identifier, null, info)); + return this; + } + + public Builder addDroppedQuery(@Nonnull final Path filePath, + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo info) { + droppedQueries.add(new QueryChange(filePath, identifier, info, null)); + return this; + } + + public Builder addPlanAndMetricsChanged(@Nonnull final Path filePath, + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo oldInfo, + @Nonnull final MetricsInfo newInfo) { + planAndMetricsChanged.add(new QueryChange(filePath, identifier, oldInfo, newInfo)); + return this; + } + + public Builder addMetricsOnlyChanged(@Nonnull final Path filePath, + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo oldInfo, + @Nonnull final MetricsInfo newInfo) { + metricsOnlyChanged.add(new QueryChange(filePath, identifier, oldInfo, newInfo)); + return this; + } + + public MetricsAnalysisResult build() { + return new MetricsAnalysisResult( + newQueries.build(), + droppedQueries.build(), + planAndMetricsChanged.build(), + metricsOnlyChanged.build(), + repositoryRoot + ); + } + } + } + + /** + * Represents a change to a query's metrics. + */ + public static class QueryChange { + public final Path filePath; + public final PlannerMetricsProto.Identifier identifier; + @Nullable + public final MetricsInfo oldInfo; + @Nullable + public final MetricsInfo newInfo; + + public QueryChange(@Nonnull final Path filePath, + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nullable final MetricsInfo oldInfo, + @Nullable final MetricsInfo newInfo) { + this.filePath = filePath; + this.identifier = identifier; + this.oldInfo = oldInfo; + this.newInfo = newInfo; + } + } +} diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java new file mode 100644 index 0000000000..c04f5841fa --- /dev/null +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java @@ -0,0 +1,84 @@ +/* + * MetricsInfo.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import com.apple.foundationdb.relational.yamltests.generated.stats.PlannerMetricsProto; + +import javax.annotation.Nonnull; +import java.nio.file.Path; +import java.util.Objects; + +public final class MetricsInfo { + @Nonnull + private final PlannerMetricsProto.Info metricsInfo; + @Nonnull + private final Path filePath; + private final int lineNumber; + + MetricsInfo(@Nonnull PlannerMetricsProto.Info metricsInfo, + @Nonnull Path filePath, + int lineNumber) { + this.metricsInfo = metricsInfo; + this.filePath = filePath; + this.lineNumber = lineNumber; + } + + @Nonnull + public PlannerMetricsProto.Info getMetricsInfo() { + return metricsInfo; + } + + @Nonnull + public Path getFilePath() { + return filePath; + } + + public int getLineNumber() { + return lineNumber; + } + + @Nonnull + public String getExplain() { + return metricsInfo.getExplain(); + } + + @Nonnull + public PlannerMetricsProto.CountersAndTimers getCountersAndTimers() { + return metricsInfo.getCountersAndTimers(); + } + + @Override + public boolean equals(final Object object) { + if (this == object) { + return true; + } + if (object == null || getClass() != object.getClass()) { + return false; + } + final MetricsInfo that = (MetricsInfo)object; + return lineNumber == that.lineNumber && Objects.equals(metricsInfo, that.metricsInfo) && Objects.equals(filePath, that.filePath); + } + + @Override + public int hashCode() { + return Objects.hash(metricsInfo, filePath, lineNumber); + } +} diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java new file mode 100644 index 0000000000..6782b3d82b --- /dev/null +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java @@ -0,0 +1,179 @@ +/* + * MetricsStatistics.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + +import javax.annotation.Nonnull; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Statistical analysis of metrics differences for a set of queries. + * Provides mean, median, standard deviation, and range calculations for each metric field. + */ +public final class MetricsStatistics { + private final Map fieldStatistics; + private final Map absoluteFieldStatistics; + + private MetricsStatistics(@Nonnull final Map fieldStatistics, + @Nonnull final Map absoluteFieldStatistics) { + this.fieldStatistics = fieldStatistics; + this.absoluteFieldStatistics = absoluteFieldStatistics; + } + + @Nonnull + public FieldStatistics getFieldStatistics(@Nonnull final String fieldName) { + return fieldStatistics.getOrDefault(fieldName, FieldStatistics.EMPTY); + } + + @Nonnull + public FieldStatistics getAbsoluteFieldStatistics(@Nonnull final String fieldName) { + return absoluteFieldStatistics.getOrDefault(fieldName, FieldStatistics.EMPTY); + } + + /** + * Statistics for a single metrics field across all queries. + */ + public static class FieldStatistics { + public static final FieldStatistics EMPTY = new FieldStatistics(ImmutableList.of(), 0.0, 0.0); + + public final List sortedValues; + public final double mean; + public final double standardDeviation; + + private FieldStatistics(final List sortedValues, + final double mean, + final double standardDeviation) { + this.sortedValues = ImmutableList.copyOf(sortedValues); + this.mean = mean; + this.standardDeviation = standardDeviation; + } + + public boolean hasChanges() { + return !sortedValues.isEmpty(); + } + + public int getChangedCount() { + return sortedValues.size(); + } + + public double getMean() { + return mean; + } + + public long getMin() { + return hasChanges() ? sortedValues.get(0) : 0L; + } + + public long getMax() { + return hasChanges() ? sortedValues.get(sortedValues.size() - 1) : 0L; + } + + public double getStandardDeviation() { + return standardDeviation; + } + + /** + * Returns the value for which {@code quantile} proportion of elements + * are smaller. This is a generalization of the concept of "percentiles" + * to real numbers. + * + * @param quantile the quantile to calculate + * @return a value that separates the smallest {@code quantile} proportion of elements + */ + public long getQuantile(double quantile) { + if (sortedValues.isEmpty()) { + return 0L; + } else { + int index = (int)(getChangedCount() * quantile); + return sortedValues.get(index); + } + } + + public long getMedian() { + return getQuantile(0.5); + } + + public long getP95() { + return getQuantile(0.95); + } + + public long getP05() { + return getQuantile(0.05); + } + } + + public static Builder newBuilder() { + return new Builder(); + } + + /** + * Builder for collecting metric differences and calculating statistics. + */ + public static class Builder { + private final Map> differences = new HashMap<>(); + private final Map> absoluteDifferences = new HashMap<>(); + + @Nonnull + public Builder addDifference(@Nonnull final String fieldName, final long difference) { + differences.computeIfAbsent(fieldName, k -> new ArrayList<>()).add(difference); + absoluteDifferences.computeIfAbsent(fieldName, k -> new ArrayList<>()).add(Math.abs(difference)); + return this; + } + + private Map buildStats(@Nonnull final Map> baseMap) { + final ImmutableMap.Builder builder = ImmutableMap.builder(); + + for (final var entry : baseMap.entrySet()) { + final var fieldName = entry.getKey(); + final var values = entry.getValue(); + + if (values.isEmpty()) { + continue; + } + + // Sort values for median calculation + Collections.sort(values); + + // Calculate statistics + final var mean = values.stream().mapToLong(Long::longValue).average().orElse(0.0); + final var variance = values.stream() + .mapToDouble(v -> Math.pow(v - mean, 2)) + .average().orElse(0.0); + final var standardDeviation = Math.sqrt(variance); + + builder.put(fieldName, new FieldStatistics(values, mean, standardDeviation)); + } + + return builder.build(); + } + + @Nonnull + public MetricsStatistics build() { + return new MetricsStatistics(buildStats(differences), buildStats(absoluteDifferences)); + } + } +} diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java index 857065c57a..dc43e21302 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java @@ -20,6 +20,7 @@ package com.apple.foundationdb.relational.yamltests; +import com.apple.foundationdb.record.logging.KeyValueLogMessage; import com.apple.foundationdb.relational.api.Options; import com.apple.foundationdb.relational.api.exceptions.ErrorCode; import com.apple.foundationdb.relational.api.exceptions.RelationalException; @@ -29,22 +30,27 @@ import com.google.common.base.Verify; import com.google.common.collect.ImmutableMap; import com.google.common.collect.LinkedListMultimap; +import com.google.protobuf.Descriptors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.junit.jupiter.api.Assertions; import org.opentest4j.TestAbortedException; import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.LoaderOptions; import org.yaml.snakeyaml.Yaml; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.io.BufferedInputStream; import java.io.BufferedReader; +import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStreamReader; import java.io.PrintWriter; import java.net.URI; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.Comparator; @@ -63,6 +69,19 @@ public final class YamlExecutionContext { private static final Logger logger = LogManager.getLogger(YamlRunner.class); + /** + * List of metrics field names that are tracked for planner comparison. + * These are the core metrics that should be consistent between runs, excluding timing + * information which can vary. + */ + public static final List TRACKED_METRIC_FIELDS = List.of( + "task_count", + "transform_count", + "transform_yield_count", + "insert_new_count", + "insert_reused_count" + ); + public static final ContextOption OPTION_FORCE_CONTINUATIONS = new ContextOption<>("optionForceContinuation"); public static final ContextOption OPTION_CORRECT_EXPLAIN = new ContextOption<>("optionCorrectExplain"); public static final ContextOption OPTION_CORRECT_METRICS = new ContextOption<>("optionCorrectMetrics"); @@ -494,6 +513,182 @@ private static ImmutableMap info + * @throws RelationalException if file cannot be read or parsed + */ + @Nonnull + @SuppressWarnings("unchecked") + public static Map loadMetricsFromYamlFile(@Nonnull final Path filePath) throws RelationalException { + final ImmutableMap.Builder resultMapBuilder = ImmutableMap.builder(); + final Map seen = new HashMap<>(); + if (!Files.exists(filePath)) { + return resultMapBuilder.build(); + } + + try { + final LoaderOptions loaderOptions = new LoaderOptions(); + loaderOptions.setAllowDuplicateKeys(true); + final var yaml = new Yaml(new CustomYamlConstructor(loaderOptions)); + final var document = yaml.load(new BufferedInputStream(new FileInputStream(filePath.toFile()))); + + if (!(document instanceof Map)) { + return resultMapBuilder.build(); + } + + final var data = (Map>>) document; + + // Parse each block in the YAML file + for (final var blockEntry : data.entrySet()) { + final var blockName = blockEntry.getKey(); + final var queries = blockEntry.getValue(); + + if (queries == null) { + continue; + } + + // Process each query in the block + for (final var queryMap : queries) { + if (queryMap == null) { + continue; + } + + // Extract line number from the "query" key if it's a LinedObject + int lineNumber = 1; // Default line number + String query = null; + String explain = null; + + for (final var entry : queryMap.entrySet()) { + final Object key = entry.getKey(); + if (key instanceof CustomYamlConstructor.LinedObject) { + CustomYamlConstructor.LinedObject linedObject = (CustomYamlConstructor.LinedObject) key; + final String keyString = (String) linedObject.getObject(); + if (keyString.equals("query")) { + query = (String) entry.getValue(); + lineNumber = ((CustomYamlConstructor.LinedObject) key).getLineNumber(); + } else if (keyString.equals("explain")) { + explain = (String) entry.getValue(); + } + } + } + + if (query != null) { + processQueryAtLine(queryMap, blockName, lineNumber, query, explain, seen, resultMapBuilder, filePath); + } + } + } + + return resultMapBuilder.build(); + } catch (final IOException e) { + throw new RelationalException(ErrorCode.INTERNAL_ERROR, e); + } + } + + /** + * Processes a single query with its line number information. + */ + @SuppressWarnings("unchecked") + private static void processQueryAtLine(Map queryMap, + String blockName, + int lineNumber, + String query, + @Nullable String explain, + Map seen, + ImmutableMap.Builder resultMapBuilder, + Path filePath) { + // Extract the query string, handling LinedObject if present + final var setup = (List) queryMap.get("setup"); + + // Build identifier + final var identifierBuilder = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName(blockName) + .setQuery(query); + if (setup != null) { + identifierBuilder.addAllSetups(setup); + } + final var identifier = identifierBuilder.build(); + + // Build counters and timers + final var countersAndTimers = PlannerMetricsProto.CountersAndTimers.newBuilder() + .setTaskCount(getLongValue(queryMap, "task_count")) + .setTaskTotalTimeNs(TimeUnit.MILLISECONDS.toNanos(getLongValue(queryMap, "task_total_time_ms"))) + .setTransformCount(getLongValue(queryMap, "transform_count")) + .setTransformTimeNs(TimeUnit.MILLISECONDS.toNanos(getLongValue(queryMap, "transform_time_ms"))) + .setTransformYieldCount(getLongValue(queryMap, "transform_yield_count")) + .setInsertTimeNs(TimeUnit.MILLISECONDS.toNanos(getLongValue(queryMap, "insert_time_ms"))) + .setInsertNewCount(getLongValue(queryMap, "insert_new_count")) + .setInsertReusedCount(getLongValue(queryMap, "insert_reused_count")) + .build(); + + // Build info + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain(explain) + .setCountersAndTimers(countersAndTimers) + .build(); + + // Check for duplicates + final var oldInfo = seen.get(identifier); + if (oldInfo == null) { + seen.put(identifier, info); + resultMapBuilder.put(identifier, new MetricsInfo(info, filePath, lineNumber)); + } else if (!info.equals(oldInfo)) { + logger.warn(KeyValueLogMessage.of("Metrics file contains multiple copies of the same query", + "file", filePath, + "block", identifier.getBlockName(), + "query", identifier.getQuery(), + "line", lineNumber)); + } + } + + /** + * Helper method to safely extract long values from YAML data. + */ + private static long getLongValue(Map map, String key) { + final var value = map.get(key); + if (value instanceof Number) { + return ((Number) value).longValue(); + } + throw new IllegalArgumentException("Expected numeric value for key: " + key + ", got: " + value); + } + + /** + * Compares two CountersAndTimers and determines if any of the tracked metrics are different. + * This method checks the core metrics that are used for planner comparison but excludes timing + * information as those can vary between runs. + * + * @param expected the expected metrics values + * @param actual the actual metrics values + * @return true if any of the tracked metrics differ + */ + public static boolean areMetricsDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, + @Nonnull final PlannerMetricsProto.CountersAndTimers actual) { + final var metricsDescriptor = expected.getDescriptorForType(); + + return TRACKED_METRIC_FIELDS.stream() + .map(metricsDescriptor::findFieldByName) + .anyMatch(field -> isMetricDifferent(expected, actual, field)); + } + + /** + * Compares a specific metric field between expected and actual values. + * + * @param expected the expected metrics + * @param actual the actual metrics + * @param fieldDescriptor the field to compare + * @return true if the metric values differ + */ + public static boolean isMetricDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, + @Nonnull final PlannerMetricsProto.CountersAndTimers actual, + @Nonnull final Descriptors.FieldDescriptor fieldDescriptor) { + final long expectedMetric = (long) expected.getField(fieldDescriptor); + final long actualMetric = (long) actual.getField(fieldDescriptor); + return expectedMetric != actualMetric; + } + @Nonnull private static String metricsBinaryProtoFileName(@Nonnull final String resourcePath) { return baseName(resourcePath) + ".metrics.binpb"; diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java index 2a0e58db2f..3fa5bff9a1 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java @@ -206,26 +206,9 @@ private void checkMetrics(final @Nonnull String currentQuery, private boolean areMetricsDifferent(final PlannerMetricsProto.CountersAndTimers expectedCountersAndTimers, final PlannerMetricsProto.CountersAndTimers actualCountersAndTimers, final Descriptors.Descriptor metricsDescriptor) { - return isMetricDifferent(expectedCountersAndTimers, - actualCountersAndTimers, - metricsDescriptor.findFieldByName("task_count"), - lineNumber) | - isMetricDifferent(expectedCountersAndTimers, - actualCountersAndTimers, - metricsDescriptor.findFieldByName("transform_count"), - lineNumber) | - isMetricDifferent(expectedCountersAndTimers, - actualCountersAndTimers, - metricsDescriptor.findFieldByName("transform_yield_count"), - lineNumber) | - isMetricDifferent(expectedCountersAndTimers, - actualCountersAndTimers, - metricsDescriptor.findFieldByName("insert_new_count"), - lineNumber) | - isMetricDifferent(expectedCountersAndTimers, - actualCountersAndTimers, - metricsDescriptor.findFieldByName("insert_reused_count"), - lineNumber); + return YamlExecutionContext.TRACKED_METRIC_FIELDS.stream() + .map(metricsDescriptor::findFieldByName) + .anyMatch(field -> isMetricDifferent(expectedCountersAndTimers, actualCountersAndTimers, field, lineNumber)); } private static boolean isMetricDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java new file mode 100644 index 0000000000..2e42a95573 --- /dev/null +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java @@ -0,0 +1,404 @@ +/* + * MetricsDiffAnalyzerTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import com.apple.foundationdb.relational.yamltests.generated.stats.PlannerMetricsProto; +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Test; + +import javax.annotation.Nonnull; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; + +class MetricsDiffAnalyzerTest { + + @Nonnull + private static final PlannerMetricsProto.CountersAndTimers BASE_DUMMY_COUNTERS = PlannerMetricsProto.CountersAndTimers.newBuilder() + .setTaskCount(10) + .setTaskTotalTimeNs(1000000L) + .setTransformCount(5) + .setTransformTimeNs(500000L) + .setTransformYieldCount(1) + .setInsertTimeNs(100000L) + .setInsertNewCount(2) + .setInsertReusedCount(3) + .build(); + + @Test + void testLoadMetricsFromYamlFile() throws Exception { + // Load test metrics file + final var testFile = getTestResourcePath("test-base.metrics.yaml"); + final var metrics = YamlExecutionContext.loadMetricsFromYamlFile(testFile); + + assertThat(metrics).hasSize(4); // 3 basic + 1 complex query + + // Verify one of the loaded metrics + final var basicIdentifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("basic_queries") + .setQuery("SELECT id FROM users WHERE id = ?") + .build(); + + assertThat(metrics).containsKey(basicIdentifier); + final var metricsInfo = metrics.get(basicIdentifier); + assertThat(metricsInfo.getExplain()).isEqualTo("Index(users_by_id [[?, ?]])"); + assertThat(metricsInfo.getCountersAndTimers().getTaskCount()).isEqualTo(10); + assertThat(metricsInfo.getCountersAndTimers().getTransformCount()).isEqualTo(5); + } + + @Test + void testCompareMetricsDetectsChanges() throws Exception { + // Load base and head metrics + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-head.metrics.yaml")); + + // Create analyzer and run comparison + final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var filePath = Paths.get("test.metrics.yaml"); + + analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); + final var result = analysisBuilder.build(); + + // Verify we detect changes + assertThat(result.getNewQueries()).hasSize(1); // "SELECT email FROM users WHERE email = ?" + assertThat(result.getDroppedQueries()).isEmpty(); + assertThat(result.getPlanAndMetricsChanged()).hasSize(2); // two queries have plan change + assertThat(result.getMetricsOnlyChanged()).hasSize(1); // users query has only metrics changes + + // Verify specific changes + final var newQuery = result.getNewQueries().get(0); + assertThat(newQuery.identifier.getQuery()).contains("SELECT email FROM users WHERE email = ?"); + + for (MetricsDiffAnalyzer.QueryChange planChanged : result.getPlanAndMetricsChanged()) { + assertThat(planChanged.newInfo).isNotNull(); + assertThat(planChanged.oldInfo).isNotNull(); + final String query = planChanged.identifier.getQuery(); + final String newExplain = planChanged.newInfo.getExplain(); + final String oldExplain = planChanged.oldInfo.getExplain(); + + if (query.contains("SELECT * FROM orders WHERE customer_id = ?")) { + assertThat(newExplain).contains("Covering_Index"); + assertThat(oldExplain).contains("Index(orders_by_customer"); + } else if (query.contains("SELECT name FROM users WHERE name LIKE 'John%'")) { + assertThat(newExplain).contains("Index(users_by_name [[John, John~]]) [IMPROVED]"); + assertThat(oldExplain).contains("Index(users_by_name [[John, John~]])"); + } else { + fail("Unexpected query: " + query); + } + } + + final var metricsChanged = result.getMetricsOnlyChanged().get(0); + assertThat(metricsChanged.newInfo).isNotNull(); + assertThat(metricsChanged.oldInfo).isNotNull(); + assertThat(metricsChanged.newInfo.getExplain()) + .isEqualTo(metricsChanged.oldInfo.getExplain()) + .contains("GroupBy(Join(Index(users), Index(orders_by_customer)))"); + } + + @Test + void testStatisticalAnalysis() { + // Create test data with known statistical properties + final var baseMetrics = createTestMetricsWithStatistics(); + final var headMetrics = createModifiedTestMetrics(); + + final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var filePath = Paths.get("test.metrics.yaml"); + + analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); + final var result = analysisBuilder.build(); + + // Generate report and verify statistical analysis + final var report = result.generateReport(); + + assertThat(report) + .contains("Statistical Summary") + .contains("Average change:") + .contains("Median change:") + .contains("Standard deviation:") + .contains("Range:") + .contains("Queries affected:"); + } + + @Test + void testOutlierDetection() { + // Create metrics with one clear outlier + final var baseMetrics = createMetricsWithOutliers(); + final var headMetrics = createMetricsWithOutlierChanges(); + + final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var filePath = Paths.get("test.metrics.yaml"); + + analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); + final var result = analysisBuilder.build(); + + final var report = result.generateReport(); + + // Should detect the outlier + assertThat(report) + .contains("- Plan unchanged + metrics changed: 11") + .contains("Significant Changes (Only Metrics Changed)") + .contains("outlier_query"); + } + + @Test + void testReportGeneration() throws Exception { + // Load test metrics + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-head.metrics.yaml")); + + final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var filePath = Paths.get("test.metrics.yaml"); + + analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); + final var result = analysisBuilder.build(); + + final var report = result.generateReport(); + + // Verify report structure + assertThat(report) + .contains("# Metrics Diff Analysis Report") + .contains("## Summary") + .contains("- New queries: 1") + .contains("- Dropped queries: 0") + .contains("- Plan changed + metrics changed: 2") + .contains("- Plan unchanged + metrics changed: 1") + .contains("## New Queries") + .contains("## Plan and Metrics Changed") + .contains("## Only Metrics Changed"); + } + + @Test + void testSignificantChangesDetection() { + // Test with dropped queries (should be significant) + final MetricsDiffAnalyzer.QueryChange change = createDummyQueryChange(); + final var analysisWithDropped = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() + .addDroppedQuery(change.filePath, change.identifier, change.oldInfo) + .build(); + assertThat(analysisWithDropped.hasSignificantChanges()).isTrue(); + + // Test with metrics-only changes (should be significant) + final var analysisWithMetricsOnly = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() + .addMetricsOnlyChanged(change.filePath, change.identifier, change.oldInfo, change.newInfo) + .build(); + assertThat(analysisWithMetricsOnly.hasSignificantChanges()).isTrue(); + + // Test with only new queries and plan changes (should not be significant) + final var analysisWithoutSignificant = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() + .addNewQuery(change.filePath, change.identifier, change.newInfo) + .addPlanAndMetricsChanged(change.filePath, change.identifier, change.oldInfo, change.newInfo) + .build(); + assertThat(analysisWithoutSignificant.hasSignificantChanges()).isFalse(); + } + + @Nonnull + private Path getTestResourcePath(String filename) { + final var classLoader = Thread.currentThread().getContextClassLoader(); + final var resource = classLoader.getResource("metrics-diff/" + filename); + assertNotNull(resource, "Test resource not found: metrics-diff/" + filename); + return Paths.get(resource.getPath()); + } + + @Nonnull + private Map createTestMetricsWithStatistics() { + // Create metrics with predictable statistical properties + final var builder = ImmutableMap.builder(); + + // Add several queries with varying metrics for statistics + for (int i = 1; i <= 5; i++) { + final var identifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("SELECT * FROM table" + i + " WHERE id = ?") + .build(); + + final var countersAndTimers = PlannerMetricsProto.CountersAndTimers.newBuilder() + .setTaskCount(10 * i) + .setTaskTotalTimeNs(1000000L * i) + .setTransformCount(5 * i) + .setTransformTimeNs(500000L * i) + .setTransformYieldCount(i) + .setInsertTimeNs(100000L * i) + .setInsertNewCount(2 * i) + .setInsertReusedCount(3 * i) + .build(); + + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(table" + i + "_by_id [[?, ?]])") + .setCountersAndTimers(countersAndTimers) + .build(); + + builder.put(identifier, new MetricsInfo(info, Paths.get("test.yaml"), i)); + } + + return builder.build(); + } + + @Nonnull + private Map createModifiedTestMetrics() { + // Create modified versions with predictable changes + final var builder = ImmutableMap.builder(); + + for (int i = 1; i <= 5; i++) { + final var identifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("SELECT * FROM table" + i + " WHERE id = ?") + .build(); + + // Add consistent changes: +2 to task_count, +1 to transform_count + final var countersAndTimers = PlannerMetricsProto.CountersAndTimers.newBuilder() + .setTaskCount(10 * i + 2) + .setTaskTotalTimeNs(1000000L * i) + .setTransformCount(5 * i + 1) + .setTransformTimeNs(500000L * i) + .setTransformYieldCount(i) + .setInsertTimeNs(100000L * i) + .setInsertNewCount(2 * i) + .setInsertReusedCount(3 * i) + .build(); + + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(table" + i + "_by_id [[?, ?]])") + .setCountersAndTimers(countersAndTimers) + .build(); + + builder.put(identifier, new MetricsInfo(info, Paths.get("test.yaml"), i)); + } + + return builder.build(); + } + + @Nonnull + private Map createMetricsWithOutliers() { + final var builder = ImmutableMap.builder(); + + // Add normal queries + for (int i = 1; i <= 10; i++) { + final var identifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("normal_query_" + i) + .build(); + + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(normal_index)") + .setCountersAndTimers(BASE_DUMMY_COUNTERS) + .build(); + + builder.put(identifier, new MetricsInfo(info, Paths.get("test.yaml"), i)); + } + + // Add outlier query with much different metrics + final var outlierIdentifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("outlier_query") + .build(); + + final var outlierCounters = BASE_DUMMY_COUNTERS.toBuilder() + .setTaskCount(BASE_DUMMY_COUNTERS.getTaskCount() * 10) // Much higher than normal + .setTransformCount(BASE_DUMMY_COUNTERS.getTransformCount() + 100) // Much higher than normal + .build(); + + final var outlierInfo = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(outlier_index)") + .setCountersAndTimers(outlierCounters) + .build(); + + builder.put(outlierIdentifier, new MetricsInfo(outlierInfo, Paths.get("test.yaml"), 4)); + + return builder.build(); + } + + @Nonnull + private Map createMetricsWithOutlierChanges() { + final var builder = ImmutableMap.builder(); + + // Normal queries with small changes + final PlannerMetricsProto.CountersAndTimers normalIncreaseCounters = BASE_DUMMY_COUNTERS.toBuilder() + .setTaskCount(BASE_DUMMY_COUNTERS.getTaskCount() + 1) + .setTransformCount(BASE_DUMMY_COUNTERS.getTransformCount() + 1) + .build(); + + for (int i = 1; i <= 10; i++) { + final var identifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("normal_query_" + i) + .build(); + + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(normal_index)") + .setCountersAndTimers(normalIncreaseCounters) + .build(); + + builder.put(identifier, new MetricsInfo(info, Paths.get("test.yaml"), i)); + } + + // Outlier query with huge changes + final var outlierIdentifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("outlier_query") + .build(); + + final var outlierCounters = BASE_DUMMY_COUNTERS.toBuilder() + .setTaskCount(BASE_DUMMY_COUNTERS.getTaskCount() * 10 + 100) // +100 change (much larger than others) + .setTransformCount(BASE_DUMMY_COUNTERS.getTransformCount() + 50) // -50 change (much larger than others) + .build(); + + final var outlierInfo = PlannerMetricsProto.Info.newBuilder() + .setExplain("Index(outlier_index)") + .setCountersAndTimers(outlierCounters) + .build(); + + builder.put(outlierIdentifier, new MetricsInfo(outlierInfo, Paths.get("test.yaml"), 4)); + + return builder.build(); + } + + @Nonnull + private MetricsDiffAnalyzer.QueryChange createDummyQueryChange() { + final var identifier = PlannerMetricsProto.Identifier.newBuilder() + .setBlockName("test_block") + .setQuery("dummy_query") + .build(); + final var info = PlannerMetricsProto.Info.newBuilder() + .setExplain("dummy_explain") + .setCountersAndTimers(BASE_DUMMY_COUNTERS) + .build(); + + final var metricsInfo = new MetricsInfo(info, Paths.get("test.yaml"), 1); + + return new MetricsDiffAnalyzer.QueryChange( + Paths.get("test.metrics.yaml"), + identifier, + metricsInfo, + metricsInfo + ); + } +} diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java new file mode 100644 index 0000000000..38d806fe08 --- /dev/null +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java @@ -0,0 +1,176 @@ +/* + * MetricsDiffIntegrationTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import org.junit.jupiter.api.Test; + +import javax.annotation.Nonnull; +import java.nio.file.Path; +import java.nio.file.Paths; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Integration tests for the complete metrics diff analysis workflow. + */ +class MetricsDiffIntegrationTest { + + @Test + void testCompleteWorkflowWithStatistics() throws Exception { + // Load test resources + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("metrics-diff-test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("metrics-diff-test-head.metrics.yaml")); + final Path basePath = Paths.get("."); + + final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); + + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); + analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("test.metrics.yaml"), analysisBuilder); + final var result = analysisBuilder.build(); + + // Verify the analysis detected expected changes + assertThat(result.getNewQueries()).hasSize(1); // table4 is new + assertThat(result.getDroppedQueries()).isEmpty(); + assertThat(result.getPlanAndMetricsChanged()).isEmpty(); // no plan changes + assertThat(result.getMetricsOnlyChanged()).hasSize(3); // table1, table2, table3 have metrics changes + + // Generate and verify report + final var report = result.generateReport(); + assertThat(report) + .contains("# Metrics Diff Analysis Report") + .contains("## Summary") + .contains("- New queries: 1") + .contains("- Dropped queries: 0") + .contains("- Plan changed + metrics changed: 0") + .contains("- Plan unchanged + metrics changed: 3") + // Verify statistical analysis + .contains("## Only Metrics Changed") + .contains("### Statistical Summary (Only Metrics Changed)") + .contains("**`task_count`**:") + .contains("Average change: 2.0") // All queries have +2 task_count + .contains("**`transform_count`**:") + .contains("Average change: 1.0"); // All queries have +1 transform_count + + // Test significant changes detection - should be true due to metrics-only changes + assertThat(result.hasSignificantChanges()).isTrue(); + } + + @Test + void testOutlierDetection() throws Exception { + // Load test resources with outlier data + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("metrics-outlier-test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("metrics-outlier-test-head.metrics.yaml")); + final Path basePath = Paths.get("."); + + final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); + + analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("outlier-test.metrics.yaml"), analysisBuilder); + final var result = analysisBuilder.build(); + + final var report = result.generateReport(); + + assertThat(report) + // Should detect the outlier query + .contains("### Significant Changes (Only Metrics Changed)") + .contains("outlier_query") + // Normal queries should have small changes (+1), outlier should have large changes (+100, +50) + .contains("`task_count`: 100 -> 200 (+100)") + .contains("`transform_count`: 50 -> 100 (+50)"); + } + + @Test + void testReportFormatting() throws Exception { + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-head.metrics.yaml")); + final Path basePath = Paths.get("."); + + final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); + + analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("format-test.metrics.yaml"), analysisBuilder); + final var result = analysisBuilder.build(); + + final var report = result.generateReport(); + + assertThat(report) + // Verify markdown formatting + .startsWith("# Metrics Diff Analysis Report") + .contains("## Summary") + .contains("## New Queries") + .contains("## Plan and Metrics Changed") + .contains("## Only Metrics Changed") + // Verify statistical formatting patterns + .containsPattern("\\*\\*`\\w+`\\*\\*:") + .containsPattern("- Average change: -?\\d+\\.\\d+") + .containsPattern("- Average absolute change: -?\\d+\\.\\d+") + .containsPattern("- Median change: -?\\d+") + .containsPattern("- Median absolute change: -?\\d++") + .containsPattern("- Standard deviation: \\d+\\.\\d+") + .containsPattern("- Standard absolute deviation: \\d+\\.\\d+") + .containsPattern("- Range: -?\\d+ to -?\\d+") + .containsPattern("- Range of absolute values: -?\\d+ to -?\\d+") + .containsPattern("- Queries affected: \\d+"); + } + + @Test + void testEmptyResultsReporting() throws Exception { + // Test with identical files (no changes) + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-base.metrics.yaml")); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath("test-base.metrics.yaml")); // Same file + final Path basePath = Paths.get("."); + + final var analyzer = new MetricsDiffAnalyzer("main", basePath); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); + + analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("no-changes.metrics.yaml"), analysisBuilder); + final var result = analysisBuilder.build(); + + final var report = result.generateReport(); + + // Should show no changes + assertThat(report) + .contains("- New queries: 0") + .contains("- Dropped queries: 0") + .contains("- Plan changed + metrics changed: 0") + .contains("- Plan unchanged + metrics changed: 0"); + + // Should not be significant + assertThat(result.hasSignificantChanges()).isFalse(); + } + + @Nonnull + private Path getTestResourcePath(String filename) { + final var classLoader = Thread.currentThread().getContextClassLoader(); + final var resource = classLoader.getResource("metrics-diff/" + filename); + assertNotNull(resource, "Test resource not found: metrics-diff/" + filename); + return Paths.get(resource.getPath()); + } +} diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsStatisticsTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsStatisticsTest.java new file mode 100644 index 0000000000..9ff57fc24f --- /dev/null +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsStatisticsTest.java @@ -0,0 +1,234 @@ +/* + * MetricsStatisticsTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.relational.yamltests; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.data.Offset.offset; + +class MetricsStatisticsTest { + + @Test + void testEmptyStatistics() { + final var stats = MetricsStatistics.newBuilder().build(); + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats).isEqualTo(MetricsStatistics.FieldStatistics.EMPTY); + assertThat(fieldStats.hasChanges()).isFalse(); + assertThat(fieldStats.getChangedCount()).isZero(); + assertThat(fieldStats.getMean()).isZero(); + assertThat(fieldStats.getMedian()).isZero(); + assertThat(fieldStats.getP05()).isZero(); + assertThat(fieldStats.getP95()).isZero(); + assertThat(fieldStats.getQuantile(0.1)).isZero(); + assertThat(fieldStats.getStandardDeviation()).isZero(); + assertThat(fieldStats.getMin()).isZero(); + assertThat(fieldStats.getMax()).isZero(); + } + + @Test + void testSingleValueStatistics() { + final var stats = MetricsStatistics.newBuilder() + .addDifference("task_count", 10L) + .build(); + + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.hasChanges()).isTrue(); + assertThat(fieldStats.getChangedCount()).isOne(); + assertThat(fieldStats.getMean()).isEqualTo(10.0); + assertThat(fieldStats.getMedian()).isEqualTo(10); + assertThat(fieldStats.getStandardDeviation()).isZero(); // Single value has no deviation + assertThat(fieldStats.getP05()).isEqualTo(10L); + assertThat(fieldStats.getP95()).isEqualTo(10L); + assertThat(fieldStats.getQuantile(0.1)).isEqualTo(10L); + assertThat(fieldStats.getMin()).isEqualTo(10L); + assertThat(fieldStats.getMax()).isEqualTo(10L); + } + + @Test + void testMultipleValuesStatistics() { + // Add values: 2, 4, 6, 8, 10 + // Mean = 6.0, Median = 6.0, StdDev = sqrt(8) ≈ 2.83 + final var stats = MetricsStatistics.newBuilder() + .addDifference("task_count", 2L) + .addDifference("task_count", 4L) + .addDifference("task_count", 6L) + .addDifference("task_count", 8L) + .addDifference("task_count", 10L) + .build(); + + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.hasChanges()).isTrue(); + assertThat(fieldStats.getChangedCount()).isEqualTo(5); + assertThat(fieldStats.getMean()).isEqualTo(6.0); + assertThat(fieldStats.getMedian()).isEqualTo(6L); + assertThat(fieldStats.getStandardDeviation()).isCloseTo(2.83, offset(0.01)); + assertThat(fieldStats.getP05()).isEqualTo(2L); + assertThat(fieldStats.getP95()).isEqualTo(10L); + assertThat(fieldStats.getQuantile(0.2)).isEqualTo(4L); + assertThat(fieldStats.getMin()).isEqualTo(2L); + assertThat(fieldStats.getMax()).isEqualTo(10L); + } + + @Test + void testEvenNumberOfValuesMedian() { + // Add values: 1, 3, 5, 7 + // Median should be (3 + 5) / 2 = 4.0 + final var stats = MetricsStatistics.newBuilder() + .addDifference("task_count", 1L) + .addDifference("task_count", 3L) + .addDifference("task_count", 5L) + .addDifference("task_count", 7L) + .build(); + + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.getMedian()).isEqualTo(5L); + assertThat(fieldStats.getMean()).isEqualTo(4.0); + } + + @Test + void testNegativeValues() { + // Add both positive and negative values: -10, -5, 0, 5, 10 + // Mean = 0.0, Median = 0.0 + final var stats = MetricsStatistics.newBuilder() + .addDifference("task_count", -10L) + .addDifference("task_count", -5L) + .addDifference("task_count", 0L) + .addDifference("task_count", 5L) + .addDifference("task_count", 10L) + .build(); + + // For field statistics, the sign matters, and the positive and negative + // cancel out + final var fieldStats = stats.getFieldStatistics("task_count"); + assertThat(fieldStats.getMean()).isZero(); + assertThat(fieldStats.getMedian()).isZero(); + assertThat(fieldStats.getMin()).isEqualTo(-10L); + assertThat(fieldStats.getMax()).isEqualTo(10L); + + // For absolute field statistics, we look at the absolute value and so the + // sign is ignored + final var absoluteFieldStats = stats.getAbsoluteFieldStatistics("task_count"); + assertThat(absoluteFieldStats.getMean()).isCloseTo(6.0, offset(0.01)); + assertThat(absoluteFieldStats.getMedian()).isEqualTo(5L); + assertThat(absoluteFieldStats.getMin()).isZero(); + assertThat(absoluteFieldStats.getMax()).isEqualTo(10L); + } + + @Test + void testMultipleFields() { + final var builder = new MetricsStatistics.Builder(); + + // Add different values for different fields + builder.addDifference("task_count", 10L); + builder.addDifference("task_count", 20L); + + builder.addDifference("transform_count", 5L); + builder.addDifference("transform_count", 15L); + builder.addDifference("transform_count", 25L); + + final var stats = builder.build(); + + // Check task_count statistics + final var taskStats = stats.getFieldStatistics("task_count"); + assertThat(taskStats.getChangedCount()).isEqualTo(2); + assertThat(taskStats.getMean()).isCloseTo(15.0, offset(0.01)); + assertThat(taskStats.getMedian()).isEqualTo(20L); + + // Check transform_count statistics + final var transformStats = stats.getFieldStatistics("transform_count"); + assertThat(transformStats.getChangedCount()).isEqualTo(3); + assertThat(transformStats.getMean()).isCloseTo(15.0, offset(0.01)); + assertThat(transformStats.getMedian()).isEqualTo(15L); + + // Check non-existent field + final var emptyStats = stats.getFieldStatistics("non_existent"); + assertThat(emptyStats).isEqualTo(MetricsStatistics.FieldStatistics.EMPTY); + } + + @Test + void testLargeDataset() { + final var builder = new MetricsStatistics.Builder(); + + // Add 1000 values from 1 to 1000 + for (long i = 1; i <= 1000; i++) { + builder.addDifference("task_count", i); + } + + final var stats = builder.build(); + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.getChangedCount()).isEqualTo(1000); + assertThat(fieldStats.getMean()).isCloseTo(500.5, offset(0.1)); + assertThat(fieldStats.getMedian()).isEqualTo(501L); + assertThat(fieldStats.getMin()).isOne(); + assertThat(fieldStats.getMax()).isEqualTo(1000); + + // Standard deviation for 1 to 1000 should be approximately 288.7 + assertThat(fieldStats.getStandardDeviation()).isCloseTo(288.7, offset(1.0)); + } + + @Test + void testDuplicateValues() { + final var builder = new MetricsStatistics.Builder(); + + // Add same value multiple times + builder.addDifference("task_count", 5L); + builder.addDifference("task_count", 5L); + builder.addDifference("task_count", 5L); + builder.addDifference("task_count", 10L); + builder.addDifference("task_count", 10L); + + final var stats = builder.build(); + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.getChangedCount()).isEqualTo(5); + assertThat(fieldStats.getMean()).isCloseTo(7.0, offset(0.01)); // (5+5+5+10+10)/5 = 7 + assertThat(fieldStats.getMedian()).isEqualTo(5L); // Middle value when sorted: [5,5,5,10,10] + assertThat(fieldStats.getQuantile(0.55)).isEqualTo(5L); + assertThat(fieldStats.getQuantile(0.65)).isEqualTo(10L); + assertThat(fieldStats.getMin()).isEqualTo(5L); + assertThat(fieldStats.getMax()).isEqualTo(10L); + } + + @Test + void testZeroValues() { + final var builder = new MetricsStatistics.Builder(); + + // Add zeros and non-zeros + builder.addDifference("task_count", 0L); + builder.addDifference("task_count", 0L); + builder.addDifference("task_count", 1L); + + final var stats = builder.build(); + final var fieldStats = stats.getFieldStatistics("task_count"); + + assertThat(fieldStats.getChangedCount()).isEqualTo(3); + assertThat(fieldStats.getMean()).isCloseTo(0.33, offset(0.01)); + assertThat(fieldStats.getMedian()).isZero(); + assertThat(fieldStats.getMin()).isZero(); + assertThat(fieldStats.getMax()).isOne(); + } +} diff --git a/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-base.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-base.metrics.yaml new file mode 100644 index 0000000000..d548fd5dc3 --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-base.metrics.yaml @@ -0,0 +1,34 @@ +statistics_test: + - query: "SELECT * FROM table1" + setup: [] + explain: "Index(table1_idx)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "SELECT * FROM table2" + setup: [] + explain: "Index(table2_idx)" + task_count: 20 + task_total_time_ms: 10 + transform_count: 10 + transform_time_ms: 5 + transform_yield_count: 0 + insert_time_ms: 2 + insert_new_count: 4 + insert_reused_count: 6 + - query: "SELECT * FROM table3" + setup: [] + explain: "Index(table3_idx)" + task_count: 30 + task_total_time_ms: 15 + transform_count: 15 + transform_time_ms: 7 + transform_yield_count: 0 + insert_time_ms: 3 + insert_new_count: 6 + insert_reused_count: 9 diff --git a/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-head.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-head.metrics.yaml new file mode 100644 index 0000000000..aa5ed60cc4 --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/metrics-diff-test-head.metrics.yaml @@ -0,0 +1,45 @@ +statistics_test: + - query: "SELECT * FROM table1" + setup: [] + explain: "Index(table1_idx)" + task_count: 12 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "SELECT * FROM table2" + setup: [] + explain: "Index(table2_idx)" + task_count: 22 + task_total_time_ms: 10 + transform_count: 11 + transform_time_ms: 5 + transform_yield_count: 0 + insert_time_ms: 2 + insert_new_count: 4 + insert_reused_count: 6 + - query: "SELECT * FROM table3" + setup: [] + explain: "Index(table3_idx)" + task_count: 32 + task_total_time_ms: 15 + transform_count: 16 + transform_time_ms: 7 + transform_yield_count: 0 + insert_time_ms: 3 + insert_new_count: 6 + insert_reused_count: 9 + - query: "SELECT * FROM table4" + setup: [] + explain: "Index(table4_idx)" + task_count: 40 + task_total_time_ms: 20 + transform_count: 20 + transform_time_ms: 10 + transform_yield_count: 0 + insert_time_ms: 4 + insert_new_count: 8 + insert_reused_count: 12 diff --git a/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-base.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-base.metrics.yaml new file mode 100644 index 0000000000..076f4a4bcb --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-base.metrics.yaml @@ -0,0 +1,155 @@ +outlier_test: + - query: "normal_query_1" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_2" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_3" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_4" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_5" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_6" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_7" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_8" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_9" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_10" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_11" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_12" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_13" + setup: [] + explain: "Index(normal_index)" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "outlier_query" + setup: [] + explain: "Index(outlier_index)" + task_count: 100 + task_total_time_ms: 50 + transform_count: 50 + transform_time_ms: 25 + transform_yield_count: 1 + insert_time_ms: 10 + insert_new_count: 2 + insert_reused_count: 3 diff --git a/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-head.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-head.metrics.yaml new file mode 100644 index 0000000000..f7bfd5aa8c --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/metrics-outlier-test-head.metrics.yaml @@ -0,0 +1,155 @@ +outlier_test: + - query: "normal_query_1" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_2" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_3" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_4" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_5" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_6" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_7" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_8" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_9" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_10" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_11" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_12" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "normal_query_13" + setup: [] + explain: "Index(normal_index)" + task_count: 11 + task_total_time_ms: 5 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 1 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "outlier_query" + setup: [] + explain: "Index(outlier_index)" + task_count: 200 + task_total_time_ms: 100 + transform_count: 100 + transform_time_ms: 50 + transform_yield_count: 1 + insert_time_ms: 20 + insert_new_count: 2 + insert_reused_count: 3 diff --git a/yaml-tests/src/test/resources/metrics-diff/test-base.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/test-base.metrics.yaml new file mode 100644 index 0000000000..8393f2e965 --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/test-base.metrics.yaml @@ -0,0 +1,47 @@ +basic_queries: + - query: "SELECT id FROM users WHERE id = ?" + setup: [] + explain: "Index(users_by_id [[?, ?]])" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "SELECT name FROM users WHERE name LIKE 'John%'" + setup: [] + explain: "Index(users_by_name [[John, John~]])" + task_count: 20 + task_total_time_ms: 10 + transform_count: 15 + transform_time_ms: 8 + transform_yield_count: 2 + insert_time_ms: 3 + insert_new_count: 8 + insert_reused_count: 7 + - query: "SELECT * FROM orders WHERE customer_id = ?" + setup: [] + explain: "Index(orders_by_customer [[?, ?]])" + task_count: 15 + task_total_time_ms: 7 + transform_count: 12 + transform_time_ms: 5 + transform_yield_count: 1 + insert_time_ms: 2 + insert_new_count: 5 + insert_reused_count: 7 + +complex_queries: + - query: "SELECT u.name, COUNT(*) FROM users u JOIN orders o ON u.id = o.customer_id GROUP BY u.name" + setup: [] + explain: "GroupBy(Join(Index(users), Index(orders_by_customer)))" + task_count: 50 + task_total_time_ms: 25 + transform_count: 30 + transform_time_ms: 15 + transform_yield_count: 5 + insert_time_ms: 8 + insert_new_count: 15 + insert_reused_count: 20 diff --git a/yaml-tests/src/test/resources/metrics-diff/test-head.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/test-head.metrics.yaml new file mode 100644 index 0000000000..3474837fae --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/test-head.metrics.yaml @@ -0,0 +1,58 @@ +basic_queries: + - query: "SELECT id FROM users WHERE id = ?" + setup: [] + explain: "Index(users_by_id [[?, ?]])" + task_count: 10 + task_total_time_ms: 5 + transform_count: 5 + transform_time_ms: 2 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 3 + - query: "SELECT name FROM users WHERE name LIKE 'John%'" + setup: [] + explain: "Index(users_by_name [[John, John~]]) [IMPROVED]" + task_count: 18 + task_total_time_ms: 9 + transform_count: 12 + transform_time_ms: 6 + transform_yield_count: 1 + insert_time_ms: 2 + insert_new_count: 6 + insert_reused_count: 6 + - query: "SELECT * FROM orders WHERE customer_id = ?" + setup: [] + explain: "Covering_Index(orders_by_customer_with_details [[?, ?]])" + task_count: 12 + task_total_time_ms: 6 + transform_count: 8 + transform_time_ms: 4 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 3 + insert_reused_count: 5 + - query: "SELECT email FROM users WHERE email = ?" + setup: [] + explain: "Index(users_by_email [[?, ?]])" + task_count: 8 + task_total_time_ms: 4 + transform_count: 6 + transform_time_ms: 2 + transform_yield_count: 0 + insert_time_ms: 1 + insert_new_count: 2 + insert_reused_count: 4 + +complex_queries: + - query: "SELECT u.name, COUNT(*) FROM users u JOIN orders o ON u.id = o.customer_id GROUP BY u.name" + setup: [] + explain: "GroupBy(Join(Index(users), Index(orders_by_customer)))" + task_count: 52 + task_total_time_ms: 26 + transform_count: 32 + transform_time_ms: 16 + transform_yield_count: 6 + insert_time_ms: 9 + insert_new_count: 17 + insert_reused_count: 22 diff --git a/yaml-tests/src/test/resources/metrics-diff/test-new-queries.metrics.yaml b/yaml-tests/src/test/resources/metrics-diff/test-new-queries.metrics.yaml new file mode 100644 index 0000000000..99236bb350 --- /dev/null +++ b/yaml-tests/src/test/resources/metrics-diff/test-new-queries.metrics.yaml @@ -0,0 +1,12 @@ +simple_queries: + - query: "SELECT * FROM products WHERE price > 100" + setup: [] + explain: "Index(products_by_price [[100, ∞]])" + task_count: 25 + task_total_time_ms: 12 + transform_count: 20 + transform_time_ms: 10 + transform_yield_count: 3 + insert_time_ms: 4 + insert_new_count: 10 + insert_reused_count: 15 diff --git a/yaml-tests/yaml-tests.gradle b/yaml-tests/yaml-tests.gradle index 66b1abb4bf..50628d611a 100644 --- a/yaml-tests/yaml-tests.gradle +++ b/yaml-tests/yaml-tests.gradle @@ -59,6 +59,7 @@ dependencies { implementation(libs.protobuf) implementation(libs.protobuf.util) implementation(libs.snakeyaml) + implementation(libs.jcommander) testImplementation(libs.bundles.test.impl) testRuntimeOnly(libs.bundles.test.runtime) @@ -234,6 +235,39 @@ tasks.register('runDebug', JavaExec) { systemProperty("tests.runQuick", "true") } +tasks.register('analyzeMetrics', JavaExec) { + description = 'Analyze metrics differences between git references' + group = 'verification' + + mainClass = 'com.apple.foundationdb.relational.yamltests.MetricsDiffAnalyzer' + classpath = sourceSets.main.runtimeClasspath + + // Pass along arguments to metrics analysis + args = [] + if (project.hasProperty('metricsAnalysis.baseRef')) { + // args += ['--base-ref', project.getProperty('metricsAnalysis.baseRef')] + args += ['--base-ref', '4.5.10.0'] + } + if (project.hasProperty('metricsAnalysis.output')) { + args += ['--output', project.getProperty('metricsAnalysis.output')] + } + if (project.hasProperty('metricsAnalysis.outlierQueries')) { + args += ['--outlier-queries', project.getProperty('metricsAnalysis.outlierQueries')] + } + args += ['--repository-root'] + if (project.hasProperty('metricsAnalysis.repositoryRoot')) { + args += [project.getProperty('metricsAnalysis.repositoryRoot')] + } else { + args += [rootProject.projectDir.toString()] + } + + // Make sure classes are built first + dependsOn classes + doFirst { + logger.info("Running metrics analysis with args: ${args}") + } +} + publishing { publications { library(MavenPublication) { From e968810d74e8055c96c4d96f304cdd84baa2655a Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 19:41:02 +0100 Subject: [PATCH 02/24] attempt different approach to combine strings from output --- .github/workflows/metrics_analysis.yml | 49 ++++++++++---------------- 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index fa0f9a2b20..17be3052cb 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -37,20 +37,8 @@ jobs: -PmetricsAnalysis.outlierQueries="$(pwd)/outlier-queries.txt" \ || echo "ANALYSIS_FAILED=true" >> $GITHUB_OUTPUT - # Read the output for PR comment - { - echo 'ANALYSIS_REPORT<> $GITHUB_OUTPUT - - if [[ -f "$(pwd)/outlier-queries.txt" ]] ; then - { - echo 'OUTLIERS_REPORT<> $GITHUB_OUTPUT - fi + - name: Add Report To Summary + run: cat metrics-analysis-output.txt > $GITHUB_STEP_SUMMARY - name: Check for outliers id: check-changes @@ -66,26 +54,25 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const output = `${{ steps.metrics-diff.outputs.OUTLIERS_REPORT }}`; + const fs = require('fs'); const hasSignificantChanges = '${{ steps.check-changes.outputs.SIGNIFICANT_CHANGES }}' === 'true'; - const commentBody = `## 📊 Metrics Diff Analysis - - ${output} - -
- ℹī¸ About this analysis + const commentBody = "## 📊 Metrics Diff Analysis\n\n" + + + fs.readFileSync("metrics-analysis-output.txt") + + ` +
+ ℹī¸ About this analysis - This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into: + This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into: - - **New queries**: Queries added in this PR - - **Dropped queries**: Queries removed in this PR - - **Plan changed + metrics changed**: Expected when query plans change - - **Metrics only changed**: ⚠ī¸ Potential concern - same plan but different metrics + - **New queries**: Queries added in this PR + - **Dropped queries**: Queries removed in this PR + - **Plan changed + metrics changed**: Expected when query plans change + - **Metrics only changed**: Same plan but different metrics - The last category may indicate planner regressions and should be investigated. -
- `; + The last category may indicate planner regressions and should be investigated. +
+ `; // Find existing comment const { data: comments } = await github.rest.issues.listComments({ @@ -125,7 +112,7 @@ jobs: const fs = require('fs'); // Parse the outliers report analysis output to find files with notable queries - const output = `${{ steps.metrics-diff.outputs.ANALYSIS_REPORT }}`; + const output = fs.readFileSync("outlier-queries.txt"); // Each query appears separated by a blank line const queries = output.split('\n\n'); @@ -146,7 +133,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, pull_number: context.issue.number, - body: `**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n${data}\n` + body: "**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n" + data + "\n" path: filePath, line: lineNumber, side: 'RIGHT' From 12c7d1e81c4e76732eedcce9470ec65082681c59 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 19:50:42 +0100 Subject: [PATCH 03/24] try write-all permissions --- .github/workflows/metrics_analysis.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 17be3052cb..78a5e4afc2 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -9,9 +9,7 @@ on: - 'yaml-tests/**' - '.github/workflows/metrics_analysis.yml' -permissions: - contents: read - pull-requests: write +permissions: write-all jobs: metrics-analysis: From 097a50046f815065450575790f48bc63556829df Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 19:54:12 +0100 Subject: [PATCH 04/24] Revert "try write-all permissions" This reverts commit 12c7d1e81c4e76732eedcce9470ec65082681c59. --- .github/workflows/metrics_analysis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 78a5e4afc2..17be3052cb 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -9,7 +9,9 @@ on: - 'yaml-tests/**' - '.github/workflows/metrics_analysis.yml' -permissions: write-all +permissions: + contents: read + pull-requests: write jobs: metrics-analysis: From c6107843f31688cb22ed06fe3bd7e33beef5e153 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:00:49 +0100 Subject: [PATCH 05/24] Revert "Revert "try write-all permissions"" This reverts commit 097a50046f815065450575790f48bc63556829df. This re-sets the permissions on the PRB to write-all. --- .github/workflows/metrics_analysis.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 17be3052cb..78a5e4afc2 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -9,9 +9,7 @@ on: - 'yaml-tests/**' - '.github/workflows/metrics_analysis.yml' -permissions: - contents: read - pull-requests: write +permissions: write-all jobs: metrics-analysis: From b40a3d0a2b977c673cb7f9d6280af94e51576879 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:07:20 +0100 Subject: [PATCH 06/24] move permission into the job spec --- .github/workflows/metrics_analysis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 78a5e4afc2..ceac44dae5 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -9,11 +9,11 @@ on: - 'yaml-tests/**' - '.github/workflows/metrics_analysis.yml' -permissions: write-all - jobs: metrics-analysis: runs-on: ubuntu-latest + permissions: + pull_requests: write steps: - name: Checkout code From c0e97fd0d23c804ef228a278604a3a84e6372a77 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:14:14 +0100 Subject: [PATCH 07/24] pull_requests -> pull-requests --- .github/workflows/metrics_analysis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index ceac44dae5..42f34b593a 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -13,7 +13,7 @@ jobs: metrics-analysis: runs-on: ubuntu-latest permissions: - pull_requests: write + pull-requests: write steps: - name: Checkout code From ebf46775c5a69d100b88ed7583368d67af53f1fd Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:23:53 +0100 Subject: [PATCH 08/24] add continue-on-error and write-all permissions --- .github/workflows/metrics_analysis.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 42f34b593a..939e3847a3 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -12,8 +12,7 @@ on: jobs: metrics-analysis: runs-on: ubuntu-latest - permissions: - pull-requests: write + permissions: write-all steps: - name: Checkout code @@ -49,14 +48,16 @@ jobs: - name: Comment on PR uses: actions/github-script@v7 + continue-on-error: true with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); const hasSignificantChanges = '${{ steps.check-changes.outputs.SIGNIFICANT_CHANGES }}' === 'true'; + const report = fs.readFileSync("./metrics-analysis-output.txt"); const commentBody = "## 📊 Metrics Diff Analysis\n\n" + - + fs.readFileSync("metrics-analysis-output.txt") + + report + `
ℹī¸ About this analysis @@ -104,13 +105,14 @@ jobs: - name: Add inline comments for outliers uses: actions/github-script@v7 if: steps.check-changes.outputs.SIGNIFICANT_CHANGES == 'true' + continue-on-error: true with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); // Parse the outliers report analysis output to find files with notable queries - const output = fs.readFileSync("outlier-queries.txt"); + const output = fs.readFileSync("./outlier-queries.txt"); // Each query appears separated by a blank line const queries = output.split('\n\n'); From e24fd99fa2a3238970b79605dbc81e4950244119 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:27:06 +0100 Subject: [PATCH 09/24] missing comma --- .github/workflows/metrics_analysis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 939e3847a3..a67351bec9 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -133,7 +133,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, pull_number: context.issue.number, - body: "**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n" + data + "\n" + body: "**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n" + data + "\n", path: filePath, line: lineNumber, side: 'RIGHT' From d7a8896b766bb9ffc25560c54feaaf9714bfe7d1 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Mon, 22 Sep 2025 20:32:51 +0100 Subject: [PATCH 10/24] set encoding on file reads --- .github/workflows/metrics_analysis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index a67351bec9..dc4f978f1d 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -54,7 +54,7 @@ jobs: script: | const fs = require('fs'); const hasSignificantChanges = '${{ steps.check-changes.outputs.SIGNIFICANT_CHANGES }}' === 'true'; - const report = fs.readFileSync("./metrics-analysis-output.txt"); + const report = fs.readFileSync("./metrics-analysis-output.txt", { encoding: 'utf8', flag: 'r'}); const commentBody = "## 📊 Metrics Diff Analysis\n\n" + + report @@ -112,7 +112,7 @@ jobs: const fs = require('fs'); // Parse the outliers report analysis output to find files with notable queries - const output = fs.readFileSync("./outlier-queries.txt"); + const output = fs.readFileSync("./outlier-queries.txt", { encoding: 'utf8', flag: 'r' })); // Each query appears separated by a blank line const queries = output.split('\n\n'); From 90d92eb53f4550eb3fd2dc6bf6f24d67e39cf709 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 11:32:17 +0100 Subject: [PATCH 11/24] move more report formatting to the Java code --- .github/workflows/metrics_analysis.yml | 35 +---- .../yamltests/MetricsDiffAnalyzer.java | 81 +++++++--- .../yamltests/MetricsDiffAnalyzerTest.java | 144 ++++++++---------- .../yamltests/MetricsDiffIntegrationTest.java | 10 +- 4 files changed, 133 insertions(+), 137 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index dc4f978f1d..57f905bd4d 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -26,13 +26,12 @@ jobs: - name: Run metrics diff analysis id: metrics-diff run: | - # Run the analysis and capture output + # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ - -PmetricsAnalysis.repositoryRoot="$( pwd )" \ - -PmetricsAnalysis.output="$(pwd)/metrics-analysis-output.txt" \ - -PmetricsAnalysis.outlierQueries="$(pwd)/outlier-queries.txt" \ - || echo "ANALYSIS_FAILED=true" >> $GITHUB_OUTPUT + -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ + -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ + -PmetricsAnalysis.outlierQueries="${{ github.workspace }}/outlier-queries.txt" - name: Add Report To Summary run: cat metrics-analysis-output.txt > $GITHUB_STEP_SUMMARY @@ -53,25 +52,7 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); - const hasSignificantChanges = '${{ steps.check-changes.outputs.SIGNIFICANT_CHANGES }}' === 'true'; - const report = fs.readFileSync("./metrics-analysis-output.txt", { encoding: 'utf8', flag: 'r'}); - - const commentBody = "## 📊 Metrics Diff Analysis\n\n" + - + report - + ` -
- ℹī¸ About this analysis - - This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into: - - - **New queries**: Queries added in this PR - - **Dropped queries**: Queries removed in this PR - - **Plan changed + metrics changed**: Expected when query plans change - - **Metrics only changed**: Same plan but different metrics - - The last category may indicate planner regressions and should be investigated. -
- `; + const report = fs.readFileSync("${{ github.workspace }}/metrics-analysis-output.txt", { encoding: 'utf8', flag: 'r'}); // Find existing comment const { data: comments } = await github.rest.issues.listComments({ @@ -90,7 +71,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, comment_id: existingComment.id, - body: commentBody + body: report }); } else { // Create new comment @@ -98,7 +79,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, - body: commentBody + body: report }); } @@ -112,7 +93,7 @@ jobs: const fs = require('fs'); // Parse the outliers report analysis output to find files with notable queries - const output = fs.readFileSync("./outlier-queries.txt", { encoding: 'utf8', flag: 'r' })); + const output = fs.readFileSync("${{ github.workspace }}/outlier-queries.txt", { encoding: 'utf8', flag: 'r' }); // Each query appears separated by a blank line const queries = output.split('\n\n'); diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index da32239b57..b9d68bbb39 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -43,6 +43,7 @@ import java.nio.file.Paths; import java.util.List; import java.util.Map; +import java.util.TreeMap; /** * Main analyzer for comparing metrics between different git references. @@ -314,8 +315,9 @@ public List getMetricsOnlyChanged() { return metricsOnlyChanged; } - public boolean hasSignificantChanges() { - return !droppedQueries.isEmpty() || !metricsOnlyChanged.isEmpty(); + @Nonnull + private Path relativePath(@Nonnull Path path) { + return repositoryRoot == null ? path : repositoryRoot.relativize(path); } /** @@ -323,7 +325,6 @@ public boolean hasSignificantChanges() { */ @Nonnull private String formatQueryDisplay(@Nonnull final QueryChange change) { - final var relativePath = repositoryRoot != null ? repositoryRoot.relativize(change.filePath) : change.filePath; int lineNumber = -1; if (change.newInfo != null) { lineNumber = change.newInfo.getLineNumber(); @@ -331,31 +332,51 @@ private String formatQueryDisplay(@Nonnull final QueryChange change) { lineNumber = change.oldInfo.getLineNumber(); } final String lineInfo = lineNumber > 0 ? (":" + lineNumber) : ""; - return String.format("%s%s: `%s`", relativePath, lineInfo, change.identifier.getQuery()); + return String.format("%s%s: `%s`", relativePath(change.filePath), lineInfo, change.identifier.getQuery()); } + /** + * Generate a markdown formatted report summarizing metrics differences. It contains a few + * different sections, including information on the set of queries that have been added and + * dropped, as well as modifications. This is intended to be presented as a single document + * for a reviewer to use to aid in assessing the scope of planner metrics changes. + * + * @return the contents of a report comparing query metrics + */ @Nonnull public String generateReport() { final var report = new StringBuilder(); - report.append("# Metrics Diff Analysis Report\n\n"); + report.append("# 📊 Metrics Diff Analysis Report\n\n"); - report.append("## Summary\n\n"); - report.append(String.format("- New queries: %d\n", newQueries.size())); - report.append(String.format("- Dropped queries: %d\n", droppedQueries.size())); - report.append(String.format("- Plan changed + metrics changed: %d\n", planAndMetricsChanged.size())); - report.append(String.format("- Plan unchanged + metrics changed: %d\n", metricsOnlyChanged.size())); - report.append("\n"); + report.append("## Summary\n\n") + .append(String.format("- New queries: %d\n", newQueries.size())) + .append(String.format("- Dropped queries: %d\n", droppedQueries.size())) + .append(String.format("- Plan changed + metrics changed: %d\n", planAndMetricsChanged.size())) + .append(String.format("- Plan unchanged + metrics changed: %d\n", metricsOnlyChanged.size())) + .append("\n"); if (!newQueries.isEmpty()) { - report.append("## New Queries\n\n"); + // Only list a count of new queries by file type. Having more test cases is generally a good thing, + // so we only want to get a rough overview + report.append("## New Queries\n\nCount of new queries by file:\n\n"); + Map countByFile = new TreeMap<>(); // tree map so that we sort by file name for (final var change : newQueries) { - report.append(String.format("- %s\n", formatQueryDisplay(change))); + countByFile.compute(change.filePath, (ignore, oldValue) -> oldValue == null ? 1 : oldValue + 1); + } + for (Map.Entry countEntry : countByFile.entrySet()) { + report.append("- ") + .append(relativePath(countEntry.getKey())) + .append(": ") + .append(countEntry.getValue()) + .append("\n"); } report.append("\n"); } if (!droppedQueries.isEmpty()) { - report.append("## Dropped Queries\n\n"); + // List out each individual dropped query. These represent a potential lack of coverage, and + // so we want these listed out more explicitly + report.append("## Dropped Queries\n\nThe following queries with metrics were removed:\n\n"); for (final var change : droppedQueries) { report.append(String.format("- %s\n", formatQueryDisplay(change))); } @@ -365,9 +386,29 @@ public String generateReport() { appendChangesList(report, planAndMetricsChanged, "Plan and Metrics Changed"); appendChangesList(report, metricsOnlyChanged, "Only Metrics Changed"); + report.append("
\n\n") + .append("ℹī¸ About this analysis\n\n") + .append("This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:\n\n") + .append(" - **New queries**: Queries added in this PR\n") + .append(" - **Dropped queries**: Queries removed in this PR\n") + .append(" - **Plan changed + metrics changed**: The query plan has changed along with planner metrics\n") + .append(" - **Metrics only changed**: Same plan but different metrics\n\n") + .append("The last category in particular may indicate planner regressions that should be investigated.\n\n") + .append("
\n"); + return report.toString(); } + /** + * Print out a simple list of outlier queries. This format is kept simple so that + * it can be processed by tools to generate in-line comments. Each query is separated + * by a blank line. The first line contains the file and line number as well as the query + * text, and then the metrics differences follow. This is intended to be used to generate + * a list of in-line comments to highlight each of these queries. + * + * @return a report of outlier queries + */ + @Nonnull public String generateOutlierQueryReport() { final List outliers = findAllOutliers(); if (outliers.isEmpty()) { @@ -402,7 +443,7 @@ private void appendStatisticalSummary(@Nonnull final StringBuilder report, @Nonn } } - private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull List changes, String title) { + private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull List changes, @Nonnull String title) { if (changes.isEmpty()) { return; } @@ -421,12 +462,9 @@ private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull Lis report.append("No outliers detected.\n\n"); } else { for (final var change : metricsOutliers) { - report.append(String.format("- %s", formatQueryDisplay(change))); + report.append(String.format("- %s", formatQueryDisplay(change))).append("\n"); if (change.oldInfo != null && change.newInfo != null) { - report.append("
\n"); appendMetricsDiff(report, change); - } else { - report.append("\n"); } } report.append("\n"); @@ -467,7 +505,8 @@ private MetricsStatistics calculateMetricsStatistics(@Nonnull final List findAllOutliers() { + @Nonnull + public List findAllOutliers() { return ImmutableList.builder() .addAll(findOutliers(planAndMetricsChanged)) .addAll(findOutliers(metricsOnlyChanged)) @@ -544,7 +583,7 @@ private void appendMetricsDiff(@Nonnull final StringBuilder report, if (oldValue != newValue) { final long change = newValue - oldValue; final String changeStr = change > 0 ? "+" + change : String.valueOf(change); - report.append(String.format("    `%s`: %d -> %d (%s)
\n", fieldName, oldValue, newValue, changeStr)); + report.append(String.format(" - `%s`: %d -> %d (%s)\n", fieldName, oldValue, newValue, changeStr)); } } } diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java index 2e42a95573..d262d9f141 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java @@ -27,6 +27,7 @@ import javax.annotation.Nonnull; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.List; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; @@ -68,6 +69,26 @@ void testLoadMetricsFromYamlFile() throws Exception { assertThat(metricsInfo.getCountersAndTimers().getTransformCount()).isEqualTo(5); } + @Nonnull + private MetricsDiffAnalyzer.MetricsAnalysisResult analyze(@Nonnull Map baseMetrics, + @Nonnull Map headMetrics) { + final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); + final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var filePath = Paths.get("test.metrics.yaml"); + + analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); + return analysisBuilder.build(); + } + + @Nonnull + private Path getTestResourcePath(String filename) { + final var classLoader = Thread.currentThread().getContextClassLoader(); + final var resource = classLoader.getResource("metrics-diff/" + filename); + assertNotNull(resource, "Test resource not found: metrics-diff/" + filename); + return Paths.get(resource.getPath()); + } + + @Test void testCompareMetricsDetectsChanges() throws Exception { // Load base and head metrics @@ -75,26 +96,19 @@ void testCompareMetricsDetectsChanges() throws Exception { getTestResourcePath("test-base.metrics.yaml")); final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( getTestResourcePath("test-head.metrics.yaml")); - - // Create analyzer and run comparison - final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); - final var filePath = Paths.get("test.metrics.yaml"); - - analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); - final var result = analysisBuilder.build(); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze(baseMetrics, headMetrics); // Verify we detect changes - assertThat(result.getNewQueries()).hasSize(1); // "SELECT email FROM users WHERE email = ?" - assertThat(result.getDroppedQueries()).isEmpty(); - assertThat(result.getPlanAndMetricsChanged()).hasSize(2); // two queries have plan change - assertThat(result.getMetricsOnlyChanged()).hasSize(1); // users query has only metrics changes + assertThat(analysis.getNewQueries()).hasSize(1); // "SELECT email FROM users WHERE email = ?" + assertThat(analysis.getDroppedQueries()).isEmpty(); + assertThat(analysis.getPlanAndMetricsChanged()).hasSize(2); // two queries have plan change + assertThat(analysis.getMetricsOnlyChanged()).hasSize(1); // users query has only metrics changes // Verify specific changes - final var newQuery = result.getNewQueries().get(0); + final var newQuery = analysis.getNewQueries().get(0); assertThat(newQuery.identifier.getQuery()).contains("SELECT email FROM users WHERE email = ?"); - for (MetricsDiffAnalyzer.QueryChange planChanged : result.getPlanAndMetricsChanged()) { + for (MetricsDiffAnalyzer.QueryChange planChanged : analysis.getPlanAndMetricsChanged()) { assertThat(planChanged.newInfo).isNotNull(); assertThat(planChanged.oldInfo).isNotNull(); final String query = planChanged.identifier.getQuery(); @@ -112,7 +126,7 @@ void testCompareMetricsDetectsChanges() throws Exception { } } - final var metricsChanged = result.getMetricsOnlyChanged().get(0); + final var metricsChanged = analysis.getMetricsOnlyChanged().get(0); assertThat(metricsChanged.newInfo).isNotNull(); assertThat(metricsChanged.oldInfo).isNotNull(); assertThat(metricsChanged.newInfo.getExplain()) @@ -125,16 +139,10 @@ void testStatisticalAnalysis() { // Create test data with known statistical properties final var baseMetrics = createTestMetricsWithStatistics(); final var headMetrics = createModifiedTestMetrics(); - - final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); - final var filePath = Paths.get("test.metrics.yaml"); - - analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); - final var result = analysisBuilder.build(); + final MetricsDiffAnalyzer.MetricsAnalysisResult result = analyze(baseMetrics, headMetrics); // Generate report and verify statistical analysis - final var report = result.generateReport(); + final String report = result.generateReport(); assertThat(report) .contains("Statistical Summary") @@ -145,28 +153,6 @@ void testStatisticalAnalysis() { .contains("Queries affected:"); } - @Test - void testOutlierDetection() { - // Create metrics with one clear outlier - final var baseMetrics = createMetricsWithOutliers(); - final var headMetrics = createMetricsWithOutlierChanges(); - - final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); - final var filePath = Paths.get("test.metrics.yaml"); - - analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); - final var result = analysisBuilder.build(); - - final var report = result.generateReport(); - - // Should detect the outlier - assertThat(report) - .contains("- Plan unchanged + metrics changed: 11") - .contains("Significant Changes (Only Metrics Changed)") - .contains("outlier_query"); - } - @Test void testReportGeneration() throws Exception { // Load test metrics @@ -174,19 +160,13 @@ void testReportGeneration() throws Exception { getTestResourcePath("test-base.metrics.yaml")); final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( getTestResourcePath("test-head.metrics.yaml")); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze(baseMetrics, headMetrics); - final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); - final var filePath = Paths.get("test.metrics.yaml"); - - analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); - final var result = analysisBuilder.build(); - - final var report = result.generateReport(); + final String report = analysis.generateReport(); // Verify report structure assertThat(report) - .contains("# Metrics Diff Analysis Report") + .contains("# 📊 Metrics Diff Analysis Report") .contains("## Summary") .contains("- New queries: 1") .contains("- Dropped queries: 0") @@ -198,34 +178,36 @@ void testReportGeneration() throws Exception { } @Test - void testSignificantChangesDetection() { - // Test with dropped queries (should be significant) - final MetricsDiffAnalyzer.QueryChange change = createDummyQueryChange(); - final var analysisWithDropped = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() - .addDroppedQuery(change.filePath, change.identifier, change.oldInfo) - .build(); - assertThat(analysisWithDropped.hasSignificantChanges()).isTrue(); - - // Test with metrics-only changes (should be significant) - final var analysisWithMetricsOnly = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() - .addMetricsOnlyChanged(change.filePath, change.identifier, change.oldInfo, change.newInfo) - .build(); - assertThat(analysisWithMetricsOnly.hasSignificantChanges()).isTrue(); - - // Test with only new queries and plan changes (should not be significant) - final var analysisWithoutSignificant = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder() - .addNewQuery(change.filePath, change.identifier, change.newInfo) - .addPlanAndMetricsChanged(change.filePath, change.identifier, change.oldInfo, change.newInfo) - .build(); - assertThat(analysisWithoutSignificant.hasSignificantChanges()).isFalse(); - } + void testOutlierDetection() { + // Create metrics with one clear outlier + final var baseMetrics = createMetricsWithOutliers(); + final var headMetrics = createMetricsWithOutlierChanges(); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze(baseMetrics, headMetrics); + final List outliers = analysis.findAllOutliers(); + assertThat(outliers) + .as("should have found a single outlier") + .hasSize(1); + final MetricsDiffAnalyzer.QueryChange expected = outliers.get(0); + assertThat(expected.newInfo) + .isNotNull(); + + final String report = analysis.generateReport(); + + // Should report the outlier in the larger report + assertThat(report) + .contains("- Plan unchanged + metrics changed: 11") + .contains("Significant Changes (Only Metrics Changed)") + .contains("outlier_query"); - @Nonnull - private Path getTestResourcePath(String filename) { - final var classLoader = Thread.currentThread().getContextClassLoader(); - final var resource = classLoader.getResource("metrics-diff/" + filename); - assertNotNull(resource, "Test resource not found: metrics-diff/" + filename); - return Paths.get(resource.getPath()); + // Should also be able to see the query in the outlier query report + final String outlierReport = analysis.generateOutlierQueryReport(); + assertThat(outlierReport).isNotEmpty(); + String[] outlierText = outlierReport.split("\n\n"); + assertThat(outlierText) + .hasSize(1); + assertThat(outlierText[0]) + .contains(expected.identifier.getQuery()) + .contains("" + expected.newInfo.getLineNumber()); } @Nonnull diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java index 38d806fe08..e977c81503 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java @@ -58,7 +58,7 @@ void testCompleteWorkflowWithStatistics() throws Exception { // Generate and verify report final var report = result.generateReport(); assertThat(report) - .contains("# Metrics Diff Analysis Report") + .contains("# 📊 Metrics Diff Analysis Report") .contains("## Summary") .contains("- New queries: 1") .contains("- Dropped queries: 0") @@ -71,9 +71,6 @@ void testCompleteWorkflowWithStatistics() throws Exception { .contains("Average change: 2.0") // All queries have +2 task_count .contains("**`transform_count`**:") .contains("Average change: 1.0"); // All queries have +1 transform_count - - // Test significant changes detection - should be true due to metrics-only changes - assertThat(result.hasSignificantChanges()).isTrue(); } @Test @@ -120,7 +117,7 @@ void testReportFormatting() throws Exception { assertThat(report) // Verify markdown formatting - .startsWith("# Metrics Diff Analysis Report") + .contains("# 📊 Metrics Diff Analysis Report") .contains("## Summary") .contains("## New Queries") .contains("## Plan and Metrics Changed") @@ -161,9 +158,6 @@ void testEmptyResultsReporting() throws Exception { .contains("- Dropped queries: 0") .contains("- Plan changed + metrics changed: 0") .contains("- Plan unchanged + metrics changed: 0"); - - // Should not be significant - assertThat(result.hasSignificantChanges()).isFalse(); } @Nonnull From a9c89695c8bd5df64abf791f5f641dbb3a87fc46 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 11:35:29 +0100 Subject: [PATCH 12/24] remove old reference to changed file set --- .github/workflows/metrics_analysis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 57f905bd4d..d98b87965f 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -108,7 +108,6 @@ jobs: if (match) { const [, filePath, lineNumber, query] = match; const data = query.substring(newl, query.length); - changedFiles.push({ filePath, query }); try { await github.rest.pulls.createReviewComment({ owner: context.repo.owner, From e9614e03806ae6111a1cf5924a9d79acff812640 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 14:43:47 +0100 Subject: [PATCH 13/24] make references to deleted and outlier queries links --- .github/workflows/metrics_analysis.yml | 30 ++++- .../yamltests/GitMetricsFileFinder.java | 32 +++++ .../yamltests/MetricsDiffAnalyzer.java | 91 ++++++++++--- .../yamltests/MetricsDiffAnalyzerTest.java | 4 +- .../yamltests/MetricsDiffIntegrationTest.java | 123 ++++++++++-------- yaml-tests/yaml-tests.gradle | 3 + 6 files changed, 204 insertions(+), 79 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index d98b87965f..6f9c113ea3 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -12,7 +12,9 @@ on: jobs: metrics-analysis: runs-on: ubuntu-latest - permissions: write-all + permissions: + issues: write + pull-requests: write steps: - name: Checkout code @@ -29,6 +31,7 @@ jobs: # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ + -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ -PmetricsAnalysis.outlierQueries="${{ github.workspace }}/outlier-queries.txt" @@ -90,9 +93,32 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const fs = require('fs'); + // First, delete any existing comments from previous runs + const { data: reviewComments } = await github.rest.pulls.listReviewComments({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + }); + + const metricsComments = reviewComments.filter(comment => + comment.user.type === 'Bot' && comment.body.includes('**Significant Metrics Change**') + ); + + for (const comment of metricsComments) { + try { + await github.rest.pulls.deleteReviewComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: comment.id, + }); + console.log(`Deleted previous metrics comment ${comment.id}`); + } catch (error) { + console.log(`Could not delete comment ${comment.id}: ${error.message}`); + } + } // Parse the outliers report analysis output to find files with notable queries + const fs = require('fs'); const output = fs.readFileSync("${{ github.workspace }}/outlier-queries.txt", { encoding: 'utf8', flag: 'r' }); // Each query appears separated by a blank line diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java index d50bf394d3..1dfb0915e2 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java @@ -174,4 +174,36 @@ public static Path getFileAtReference(@Nonnull final String filePath, "Failed to get file content at reference " + gitRef + ":" + filePath, ErrorCode.INTERNAL_ERROR, e); } } + + /** + * Get the current commit hash of the repo. + * + * @param repositoryRoot the directory to run the git process from + * @return the current commit hash + * @throws RelationalException if git commit fails + */ + @Nonnull + public static String getHeadReference(@Nonnull Path repositoryRoot) throws RelationalException { + try { + final var command = List.of("git", "rev-parse", "HEAD"); + final var processBuilder = new ProcessBuilder(command) + .directory(repositoryRoot.toFile()) + .redirectErrorStream(false); + + final var process = processBuilder.start(); + final var exitCode = process.waitFor(); + final String ref = new String(process.getInputStream().readAllBytes()).strip(); + + if (exitCode != 0) { + throw new RelationalException( + "Git rev-parse command failed with exit code " + exitCode, + ErrorCode.INTERNAL_ERROR); + } + + return ref; + } catch (final IOException | InterruptedException e) { + throw new RelationalException( + "Failed to get current head reference", ErrorCode.INTERNAL_ERROR, e); + } + } } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index b9d68bbb39..828281b57f 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -53,9 +53,6 @@ public final class MetricsDiffAnalyzer { private static final Logger logger = LogManager.getLogger(MetricsDiffAnalyzer.class); - private final String baseRef; - private final Path repositoryRoot; - /** * Command line arguments for the metrics diff analyzer. */ @@ -72,14 +69,30 @@ public static class Arguments { @Parameter(names = {"--outlier-queries"}, description = "Output file path to specifically list data for outlier queries", order = 3) public String outlierQueryPath; + @Parameter(names = {"--url-base"}, description = "Base of the URL to use for linking to queries", order = 4) + public String urlBase; + @Parameter(names = {"--help", "-h"}, description = "Show help message", help = true, order = 4) public boolean help; } + @Nonnull + private final String baseRef; + @Nonnull + private final String headRef; + @Nullable + private final Path repositoryRoot; + @Nullable + private final String urlBase; + public MetricsDiffAnalyzer(@Nonnull final String baseRef, - @Nonnull final Path repositoryRoot) { + @Nonnull final String headRef, + @Nullable final Path repositoryRoot, + @Nullable final String urlBase) { this.baseRef = baseRef; + this.headRef = headRef; this.repositoryRoot = repositoryRoot; + this.urlBase = urlBase; } /** @@ -110,7 +123,7 @@ public static void main(String[] args) { final Path outputPath = arguments.outputPath != null ? Paths.get(arguments.outputPath) : null; try { - final var analyzer = new MetricsDiffAnalyzer(baseRef, repositoryRoot); + final var analyzer = new MetricsDiffAnalyzer(baseRef, GitMetricsFileFinder.getHeadReference(repositoryRoot), repositoryRoot, arguments.urlBase); final var analysis = analyzer.analyze(); final var report = analysis.generateReport(); @@ -157,7 +170,7 @@ public MetricsAnalysisResult analyze() throws RelationalException { "base", baseRef, "file_count", changedFiles.size())); - final var analysisBuilder = MetricsAnalysisResult.newBuilder(repositoryRoot); + final var analysisBuilder = newAnalysisBuilder(); for (final var filePath : changedFiles) { analyzeYamlFile(filePath, analysisBuilder); @@ -276,6 +289,11 @@ public void compareMetrics(@Nonnull final Map droppedQueries; private final List planAndMetricsChanged; private final List metricsOnlyChanged; + @Nonnull + private final String baseRef; + @Nonnull + private final String headRef; @Nullable private final Path repositoryRoot; + @Nullable + private final String urlBase; private MetricsAnalysisResult(@Nonnull final List newQueries, @Nonnull final List droppedQueries, @Nonnull final List planAndMetricsChanged, @Nonnull final List metricsOnlyChanged, - @Nullable final Path repositoryRoot) { + @Nonnull final String baseRef, + @Nonnull final String headRef, + @Nullable final Path repositoryRoot, + @Nullable final String urlBase) { this.newQueries = newQueries; this.droppedQueries = droppedQueries; this.planAndMetricsChanged = planAndMetricsChanged; this.metricsOnlyChanged = metricsOnlyChanged; + this.baseRef = baseRef; + this.headRef = headRef; this.repositoryRoot = repositoryRoot; + this.urlBase = urlBase; } public List getNewQueries() { @@ -320,19 +350,35 @@ private Path relativePath(@Nonnull Path path) { return repositoryRoot == null ? path : repositoryRoot.relativize(path); } + @Nonnull + private String formatQueryDisplay(@Nonnull final QueryChange change) { + return formatQueryDisplay(change, true); + } + /** * Helper method to format a query change for display with relative path and line number. */ @Nonnull - private String formatQueryDisplay(@Nonnull final QueryChange change) { + private String formatQueryDisplay(@Nonnull final QueryChange change, boolean asLink) { int lineNumber = -1; + String ref = null; if (change.newInfo != null) { lineNumber = change.newInfo.getLineNumber(); + ref = headRef; } else if (change.oldInfo != null) { lineNumber = change.oldInfo.getLineNumber(); + ref = baseRef; } final String lineInfo = lineNumber > 0 ? (":" + lineNumber) : ""; - return String.format("%s%s: `%s`", relativePath(change.filePath), lineInfo, change.identifier.getQuery()); + String locationText = relativePath(change.filePath) + lineInfo; + if (asLink && urlBase != null) { + locationText = "[" + locationText + "](" + urlBase + "/" + ref + "/" + relativePath(change.filePath); + if (lineNumber > 0) { + locationText += "#L" + lineNumber; + } + locationText += ")"; + } + return locationText + ": `" + change.identifier.getQuery() + "`"; } /** @@ -417,7 +463,8 @@ public String generateOutlierQueryReport() { final StringBuilder report = new StringBuilder(); for (QueryChange queryChange : outliers) { - report.append(formatQueryDisplay(queryChange)).append("\n"); + // One query per line. Do not use format as a link + report.append(formatQueryDisplay(queryChange, false)).append("\n"); appendMetricsDiff(report, queryChange); report.append("\n"); } @@ -588,24 +635,25 @@ private void appendMetricsDiff(@Nonnull final StringBuilder report, } } - public static MetricsAnalysisResult.Builder newBuilder() { - return new Builder(null); - } - - public static MetricsAnalysisResult.Builder newBuilder(@Nullable final Path repositoryRoot) { - return new Builder(repositoryRoot); - } - public static class Builder { private final ImmutableList.Builder newQueries = ImmutableList.builder(); private final ImmutableList.Builder droppedQueries = ImmutableList.builder(); private final ImmutableList.Builder planAndMetricsChanged = ImmutableList.builder(); private final ImmutableList.Builder metricsOnlyChanged = ImmutableList.builder(); + @Nonnull + private final String baseRef; + @Nonnull + private final String headRef; @Nullable private final Path repositoryRoot; + @Nullable + private final String urlBase; - public Builder(@Nullable final Path repositoryRoot) { + public Builder(@Nonnull String baseRef, @Nonnull String headRef, @Nullable final Path repositoryRoot, @Nullable final String urlBase) { + this.baseRef = baseRef; + this.headRef = headRef; this.repositoryRoot = repositoryRoot; + this.urlBase = urlBase; } public Builder addNewQuery(@Nonnull final Path filePath, @@ -644,7 +692,10 @@ public MetricsAnalysisResult build() { droppedQueries.build(), planAndMetricsChanged.build(), metricsOnlyChanged.build(), - repositoryRoot + baseRef, + headRef, + repositoryRoot, + urlBase ); } } diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java index d262d9f141..b90d18da77 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java @@ -72,8 +72,8 @@ void testLoadMetricsFromYamlFile() throws Exception { @Nonnull private MetricsDiffAnalyzer.MetricsAnalysisResult analyze(@Nonnull Map baseMetrics, @Nonnull Map headMetrics) { - final var analyzer = new MetricsDiffAnalyzer("base", Paths.get(".")); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(); + final var analyzer = new MetricsDiffAnalyzer("base", "head", Paths.get("."), null); + final var analysisBuilder = analyzer.newAnalysisBuilder(); final var filePath = Paths.get("test.metrics.yaml"); analyzer.compareMetrics(baseMetrics, headMetrics, filePath, analysisBuilder); diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java index e977c81503..74572679d1 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java @@ -20,6 +20,7 @@ package com.apple.foundationdb.relational.yamltests; +import com.apple.foundationdb.relational.api.exceptions.RelationalException; import org.junit.jupiter.api.Test; import javax.annotation.Nonnull; @@ -36,31 +37,23 @@ class MetricsDiffIntegrationTest { @Test void testCompleteWorkflowWithStatistics() throws Exception { - // Load test resources - final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("metrics-diff-test-base.metrics.yaml")); - final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("metrics-diff-test-head.metrics.yaml")); - final Path basePath = Paths.get("."); - - final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); - - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); - analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("test.metrics.yaml"), analysisBuilder); - final var result = analysisBuilder.build(); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze( + "metrics-diff-test-base.metrics.yaml", + "metrics-diff-test-head.metrics.yaml"); // Verify the analysis detected expected changes - assertThat(result.getNewQueries()).hasSize(1); // table4 is new - assertThat(result.getDroppedQueries()).isEmpty(); - assertThat(result.getPlanAndMetricsChanged()).isEmpty(); // no plan changes - assertThat(result.getMetricsOnlyChanged()).hasSize(3); // table1, table2, table3 have metrics changes + assertThat(analysis.getNewQueries()).hasSize(1); // table4 is new + assertThat(analysis.getDroppedQueries()).isEmpty(); + assertThat(analysis.getPlanAndMetricsChanged()).isEmpty(); // no plan changes + assertThat(analysis.getMetricsOnlyChanged()).hasSize(3); // table1, table2, table3 have metrics changes // Generate and verify report - final var report = result.generateReport(); + final var report = analysis.generateReport(); assertThat(report) .contains("# 📊 Metrics Diff Analysis Report") .contains("## Summary") .contains("- New queries: 1") + .contains("test.metrics.yaml: 1") .contains("- Dropped queries: 0") .contains("- Plan changed + metrics changed: 0") .contains("- Plan unchanged + metrics changed: 3") @@ -74,26 +67,50 @@ void testCompleteWorkflowWithStatistics() throws Exception { } @Test - void testOutlierDetection() throws Exception { - // Load test resources with outlier data - final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("metrics-outlier-test-base.metrics.yaml")); - final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("metrics-outlier-test-head.metrics.yaml")); - final Path basePath = Paths.get("."); + void testCompleteWorkflowBackwards() throws Exception { + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze( + "metrics-diff-test-head.metrics.yaml", + "metrics-diff-test-base.metrics.yaml"); + + // Verify the analysis detected expected changes + assertThat(analysis.getNewQueries()).isEmpty(); + assertThat(analysis.getDroppedQueries()).hasSize(1); + assertThat(analysis.getPlanAndMetricsChanged()).isEmpty(); // no plan changes + assertThat(analysis.getMetricsOnlyChanged()).hasSize(3); // table1, table2, table3 have metrics changes - final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); + // Generate and verify report + final var report = analysis.generateReport(); + assertThat(report) + .contains("# 📊 Metrics Diff Analysis Report") + .contains("## Summary") + .contains("- New queries: 0") + .contains("- Dropped queries: 1") + .contains("[test.metrics.yaml:35](https://example.com/repo/blob/base/test.metrics.yaml#L35): `SELECT * FROM table4`") + .contains("- Plan changed + metrics changed: 0") + .contains("- Plan unchanged + metrics changed: 3") + // Verify statistical analysis + .contains("## Only Metrics Changed") + .contains("### Statistical Summary (Only Metrics Changed)") + .contains("**`task_count`**:") + .contains("Average change: -2.0") // All queries have +2 task_count + .contains("**`transform_count`**:") + .contains("Average change: -1.0"); // All queries have +1 transform_count + } - analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("outlier-test.metrics.yaml"), analysisBuilder); - final var result = analysisBuilder.build(); + @Test + void testOutlierDetection() throws Exception { + // Load test files with outlier case + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze( + "metrics-outlier-test-base.metrics.yaml", + "metrics-outlier-test-head.metrics.yaml"); - final var report = result.generateReport(); + final var report = analysis.generateReport(); assertThat(report) // Should detect the outlier query .contains("### Significant Changes (Only Metrics Changed)") .contains("outlier_query") + .contains("[test.metrics.yaml:145](https://example.com/repo/blob/head/test.metrics.yaml#L145): `outlier_query`") // Normal queries should have small changes (+1), outlier should have large changes (+100, +50) .contains("`task_count`: 100 -> 200 (+100)") .contains("`transform_count`: 50 -> 100 (+50)"); @@ -101,19 +118,11 @@ void testOutlierDetection() throws Exception { @Test void testReportFormatting() throws Exception { - final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("test-base.metrics.yaml")); - final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("test-head.metrics.yaml")); - final Path basePath = Paths.get("."); - - final var analyzer = new MetricsDiffAnalyzer("HEAD", basePath); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); - - analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("format-test.metrics.yaml"), analysisBuilder); - final var result = analysisBuilder.build(); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze( + "test-base.metrics.yaml", + "test-head.metrics.yaml"); - final var report = result.generateReport(); + final var report = analysis.generateReport(); assertThat(report) // Verify markdown formatting @@ -138,19 +147,8 @@ void testReportFormatting() throws Exception { @Test void testEmptyResultsReporting() throws Exception { // Test with identical files (no changes) - final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("test-base.metrics.yaml")); - final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( - getTestResourcePath("test-base.metrics.yaml")); // Same file - final Path basePath = Paths.get("."); - - final var analyzer = new MetricsDiffAnalyzer("main", basePath); - final var analysisBuilder = MetricsDiffAnalyzer.MetricsAnalysisResult.newBuilder(basePath); - - analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("no-changes.metrics.yaml"), analysisBuilder); - final var result = analysisBuilder.build(); - - final var report = result.generateReport(); + final MetricsDiffAnalyzer.MetricsAnalysisResult analysis = analyze("test-base.metrics.yaml", "test-base.metrics.yaml"); + final var report = analysis.generateReport(); // Should show no changes assertThat(report) @@ -161,7 +159,22 @@ void testEmptyResultsReporting() throws Exception { } @Nonnull - private Path getTestResourcePath(String filename) { + private static MetricsDiffAnalyzer.MetricsAnalysisResult analyze(@Nonnull String baseResource, @Nonnull String headResource) throws RelationalException { + // Load test resources + final var baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath(baseResource)); + final var headMetrics = YamlExecutionContext.loadMetricsFromYamlFile( + getTestResourcePath(headResource)); + final Path basePath = Paths.get("."); + + final MetricsDiffAnalyzer analyzer = new MetricsDiffAnalyzer("base", "head", basePath, "https://example.com/repo/blob"); + final MetricsDiffAnalyzer.MetricsAnalysisResult.Builder analysisBuilder = analyzer.newAnalysisBuilder(); + analyzer.compareMetrics(baseMetrics, headMetrics, Paths.get("test.metrics.yaml"), analysisBuilder); + return analysisBuilder.build(); + } + + @Nonnull + private static Path getTestResourcePath(String filename) { final var classLoader = Thread.currentThread().getContextClassLoader(); final var resource = classLoader.getResource("metrics-diff/" + filename); assertNotNull(resource, "Test resource not found: metrics-diff/" + filename); diff --git a/yaml-tests/yaml-tests.gradle b/yaml-tests/yaml-tests.gradle index 50628d611a..8b4567ff28 100644 --- a/yaml-tests/yaml-tests.gradle +++ b/yaml-tests/yaml-tests.gradle @@ -248,6 +248,9 @@ tasks.register('analyzeMetrics', JavaExec) { // args += ['--base-ref', project.getProperty('metricsAnalysis.baseRef')] args += ['--base-ref', '4.5.10.0'] } + if (project.hasProperty('metricsAnalysis.urlBase')) { + args += ['--url-base', project.getProperty('metricsAnalysis.urlBase')] + } if (project.hasProperty('metricsAnalysis.output')) { args += ['--output', project.getProperty('metricsAnalysis.output')] } From 60e932685aa76750d777d75de87eb429341fbb37 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 14:55:11 +0100 Subject: [PATCH 14/24] add explicit signa to metrics report --- .../yamltests/MetricsDiffAnalyzer.java | 27 ++++++++++++++----- .../yamltests/MetricsDiffIntegrationTest.java | 20 +++++++------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index 828281b57f..dbe8bbc7a0 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -471,20 +471,33 @@ public String generateOutlierQueryReport() { return report.toString(); } + @Nonnull + private String sign(double d) { + // For adding an explicit plus to positive values. Rely on usual toString of negative values + // to add the minus sign + return d > 0 ? "+" : ""; + } + + @Nonnull + private String sign(long l) { + // See: sign(double) + return l > 0 ? "+" : ""; + } + private void appendStatisticalSummary(@Nonnull final StringBuilder report, @Nonnull final MetricsStatistics stats) { for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { final var fieldStats = stats.getFieldStatistics(fieldName); final var absoluteFieldStats = stats.getAbsoluteFieldStatistics(fieldName); if (fieldStats.hasChanges() || absoluteFieldStats.hasChanges()) { report.append(String.format("**`%s`**:\n", fieldName)); - report.append(String.format(" - Average change: %.1f\n", fieldStats.getMean())); - report.append(String.format(" - Average absolute change: %.1f\n", absoluteFieldStats.getMean())); - report.append(String.format(" - Median change: %d\n", fieldStats.getMedian())); - report.append(String.format(" - Median absolute change: %d\n", absoluteFieldStats.getMedian())); + report.append(String.format(" - Average change: %s%.1f\n", sign(fieldStats.getMean()), fieldStats.getMean())); + report.append(String.format(" - Average absolute change: %s%.1f\n", sign(absoluteFieldStats.getMean()), absoluteFieldStats.getMean())); + report.append(String.format(" - Median change: %s%d\n", sign(fieldStats.getMedian()), fieldStats.getMedian())); + report.append(String.format(" - Median absolute change: %s%d\n", sign(absoluteFieldStats.getMedian()), absoluteFieldStats.getMedian())); report.append(String.format(" - Standard deviation: %.1f\n", fieldStats.getStandardDeviation())); report.append(String.format(" - Standard absolute deviation: %.1f\n", absoluteFieldStats.getStandardDeviation())); - report.append(String.format(" - Range: %d to %d\n", fieldStats.getMin(), fieldStats.getMax())); - report.append(String.format(" - Range of absolute values: %d to %d\n", absoluteFieldStats.getMin(), absoluteFieldStats.getMax())); + report.append(String.format(" - Range: %s%d to %s%d\n", sign(fieldStats.getMin()), fieldStats.getMin(), sign(fieldStats.getMax()), fieldStats.getMax())); + report.append(String.format(" - Range of absolute values: %s%d to %s%d\n", sign(absoluteFieldStats.getMin()), absoluteFieldStats.getMin(), sign(absoluteFieldStats.getMax()), absoluteFieldStats.getMax())); report.append(String.format(" - Queries affected: %d\n\n", fieldStats.getChangedCount())); } } @@ -629,7 +642,7 @@ private void appendMetricsDiff(@Nonnull final StringBuilder report, final long newValue = (long)newMetrics.getField(field); if (oldValue != newValue) { final long change = newValue - oldValue; - final String changeStr = change > 0 ? "+" + change : String.valueOf(change); + final String changeStr = sign(change) + change; report.append(String.format(" - `%s`: %d -> %d (%s)\n", fieldName, oldValue, newValue, changeStr)); } } diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java index 74572679d1..a402b7a411 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffIntegrationTest.java @@ -61,9 +61,9 @@ void testCompleteWorkflowWithStatistics() throws Exception { .contains("## Only Metrics Changed") .contains("### Statistical Summary (Only Metrics Changed)") .contains("**`task_count`**:") - .contains("Average change: 2.0") // All queries have +2 task_count + .contains("Average change: +2.0") // All queries have +2 task_count .contains("**`transform_count`**:") - .contains("Average change: 1.0"); // All queries have +1 transform_count + .contains("Average change: +1.0"); // All queries have +1 transform_count } @Test @@ -92,9 +92,9 @@ void testCompleteWorkflowBackwards() throws Exception { .contains("## Only Metrics Changed") .contains("### Statistical Summary (Only Metrics Changed)") .contains("**`task_count`**:") - .contains("Average change: -2.0") // All queries have +2 task_count + .contains("Average change: -2.0") // All queries have -2 task_count .contains("**`transform_count`**:") - .contains("Average change: -1.0"); // All queries have +1 transform_count + .contains("Average change: -1.0"); // All queries have -1 transform_count } @Test @@ -133,14 +133,14 @@ void testReportFormatting() throws Exception { .contains("## Only Metrics Changed") // Verify statistical formatting patterns .containsPattern("\\*\\*`\\w+`\\*\\*:") - .containsPattern("- Average change: -?\\d+\\.\\d+") - .containsPattern("- Average absolute change: -?\\d+\\.\\d+") - .containsPattern("- Median change: -?\\d+") - .containsPattern("- Median absolute change: -?\\d++") + .containsPattern("- Average change: [+-]?\\d+\\.\\d+") + .containsPattern("- Average absolute change: \\+?\\d+\\.\\d+") + .containsPattern("- Median change: [+-]?\\d+") + .containsPattern("- Median absolute change: \\+?\\d++") .containsPattern("- Standard deviation: \\d+\\.\\d+") .containsPattern("- Standard absolute deviation: \\d+\\.\\d+") - .containsPattern("- Range: -?\\d+ to -?\\d+") - .containsPattern("- Range of absolute values: -?\\d+ to -?\\d+") + .containsPattern("- Range: [+-]?\\d+ to [+-]?\\d+") + .containsPattern("- Range of absolute values: \\+?\\d+ to \\+?\\d+") .containsPattern("- Queries affected: \\d+"); } From cbebd41c9747c2691b18597d8c95fd5509ebf6a5 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 18:51:59 +0100 Subject: [PATCH 15/24] use commit refs for both head and base refs --- .github/workflows/metrics_analysis.yml | 3 +- .../yamltests/GitMetricsFileFinder.java | 49 ++- .../yamltests/MetricsDiffAnalyzer.java | 299 ++++++++++-------- .../relational/yamltests/MetricsInfo.java | 16 +- .../yamltests/YamlExecutionContext.java | 8 +- yaml-tests/yaml-tests.gradle | 3 + 6 files changed, 221 insertions(+), 157 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 6f9c113ea3..ce32e54f61 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -30,7 +30,8 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ + -PmetricsAnalysis.baseRef="${{ github.base_ref }}" \ + -PmetricsAnalysis.headRef="${{ github.head_ref }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java index 1dfb0915e2..c951c0e25d 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java @@ -28,9 +28,11 @@ import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -52,14 +54,16 @@ private GitMetricsFileFinder() { * Finds all metrics YAML files that have changed between two git references. * * @param baseRef the base git reference (e.g., "main", "HEAD~1", commit SHA) + * @param headRef the target git reference (e.g., "main", "HEAD~1", commit SHA) * @param repositoryRoot the root directory of the git repository * @return set of paths to changed metrics files (both .yaml and .binpb) * @throws RelationalException if git command fails or repository is not valid */ @Nonnull public static Set findChangedMetricsYamlFiles(@Nonnull final String baseRef, + @Nonnull final String headRef, @Nonnull final Path repositoryRoot) throws RelationalException { - final List changedFiles = getChangedFiles(baseRef, repositoryRoot); + final List changedFiles = getChangedFiles(baseRef, headRef, repositoryRoot); final ImmutableSet.Builder metricsFiles = ImmutableSet.builder(); for (final var filePath : changedFiles) { @@ -75,14 +79,16 @@ public static Set findChangedMetricsYamlFiles(@Nonnull final String baseRe * Gets a list of all files changed between two git references. * * @param baseRef the base reference + * @param headRef the target reference * @param repositoryRoot the repository root directory * @return list of relative file paths that have changed * @throws RelationalException if git command fails */ @Nonnull private static List getChangedFiles(@Nonnull final String baseRef, + @Nonnull final String headRef, @Nonnull final Path repositoryRoot) throws RelationalException { - final var command = List.of("git", "diff", "--name-only", baseRef); + final var command = List.of("git", "diff", "--name-only", baseRef + "..." + headRef); try { if (logger.isDebugEnabled()) { logger.debug(KeyValueLogMessage.of("Executing git command", @@ -94,12 +100,14 @@ private static List getChangedFiles(@Nonnull final String baseRef, .redirectErrorStream(true); final var process = processBuilder.start(); - final var result = new ArrayList(); - - try (final var reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) { - String line; - while ((line = reader.readLine()) != null) { - result.add(line.trim()); + final List result = new ArrayList<>(); + + try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { + for (String line = reader.readLine(); line != null; line = reader.readLine()) { + final String stripped = line.strip(); + if (!stripped.isEmpty()) { + result.add(stripped); + } } } @@ -120,6 +128,7 @@ private static List getChangedFiles(@Nonnull final String baseRef, * Checks if a file path represents a metrics file (either .yaml or .binpb). * * @param filePath the file path to check + * * @return true if the file is a metrics file */ private static boolean isMetricsYamlFile(@Nonnull final String filePath) { @@ -143,10 +152,10 @@ public static boolean fileExists(@Nonnull final Path filePath) { * @param filePath the relative file path * @param gitRef the git reference * @param repositoryRoot the repository root - * @return path to a temporary file containing the content at the specified reference + * @return path to a temporary file containing the content at the specified reference or {@code null} if it didn't exist * @throws RelationalException if git command fails */ - @Nonnull + @Nullable public static Path getFileAtReference(@Nonnull final String filePath, @Nonnull final String gitRef, @Nonnull final Path repositoryRoot) throws RelationalException { @@ -162,10 +171,14 @@ public static Path getFileAtReference(@Nonnull final String filePath, final var exitCode = process.waitFor(); if (exitCode != 0) { + // File could not be loaded. It was probably not present at that version, so return null + if (logger.isDebugEnabled()) { + logger.debug(KeyValueLogMessage.of("Unable to get file contents at reference", + "path", filePath, + "ref", gitRef)); + } Files.deleteIfExists(tempFile); - throw new RelationalException( - "Git show command failed with exit code " + exitCode + " for " + gitRef + ":" + filePath, - ErrorCode.INTERNAL_ERROR); + return null; } return tempFile; @@ -176,23 +189,25 @@ public static Path getFileAtReference(@Nonnull final String filePath, } /** - * Get the current commit hash of the repo. + * Get the commit hash for a given reference. This is to construct the commit values for use in things + * like permanent URLs. * * @param repositoryRoot the directory to run the git process from + * @param refName a git reference (e.g., branch name, "HEAD", etc.) * @return the current commit hash * @throws RelationalException if git commit fails */ @Nonnull - public static String getHeadReference(@Nonnull Path repositoryRoot) throws RelationalException { + public static String getCommitHash(@Nonnull Path repositoryRoot, @Nonnull String refName) throws RelationalException { try { - final var command = List.of("git", "rev-parse", "HEAD"); + final var command = List.of("git", "rev-parse", refName); final var processBuilder = new ProcessBuilder(command) .directory(repositoryRoot.toFile()) .redirectErrorStream(false); final var process = processBuilder.start(); final var exitCode = process.waitFor(); - final String ref = new String(process.getInputStream().readAllBytes()).strip(); + final String ref = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).strip(); if (exitCode != 0) { throw new RelationalException( diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index dbe8bbc7a0..c3af084359 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -53,6 +53,15 @@ public final class MetricsDiffAnalyzer { private static final Logger logger = LogManager.getLogger(MetricsDiffAnalyzer.class); + @Nonnull + private final String baseRef; + @Nonnull + private final String headRef; + @Nonnull + private final Path repositoryRoot; + @Nullable + private final String urlBase; + /** * Command line arguments for the metrics diff analyzer. */ @@ -60,34 +69,28 @@ public static class Arguments { @Parameter(names = {"--base-ref", "-b"}, description = "Git reference for baseline (e.g., 'main', commit SHA)", order = 0) public String baseRef = "main"; + @Parameter(names = {"--head-ref"}, description = "Git reference for new code (e.g., 'HEAD', commit SHA)", order = 0) + public String headRef = "HEAD"; + @Parameter(names = {"--repository-root", "-r"}, description = "Path to git repository root", order = 1) public String repositoryRoot = "."; @Parameter(names = {"--output", "-o"}, description = "Output file path (writes to stdout if not specified)", order = 2) public String outputPath; - @Parameter(names = {"--outlier-queries"}, description = "Output file path to specifically list data for outlier queries", order = 3) + @Parameter(names = {"--outlier-queries"}, description = "Output file path to specifically list data for outlier queries", order = 5) public String outlierQueryPath; - @Parameter(names = {"--url-base"}, description = "Base of the URL to use for linking to queries", order = 4) + @Parameter(names = {"--url-base"}, description = "Base of the URL to use for linking to queries", order = 6) public String urlBase; @Parameter(names = {"--help", "-h"}, description = "Show help message", help = true, order = 4) public boolean help; } - @Nonnull - private final String baseRef; - @Nonnull - private final String headRef; - @Nullable - private final Path repositoryRoot; - @Nullable - private final String urlBase; - public MetricsDiffAnalyzer(@Nonnull final String baseRef, @Nonnull final String headRef, - @Nullable final Path repositoryRoot, + @Nonnull final Path repositoryRoot, @Nullable final String urlBase) { this.baseRef = baseRef; this.headRef = headRef; @@ -98,6 +101,7 @@ public MetricsDiffAnalyzer(@Nonnull final String baseRef, /** * Main entry point for command-line usage. */ + @SuppressWarnings("PMD.SystemPrintln") public static void main(String[] args) { final Arguments arguments = new Arguments(); final JCommander commander = JCommander.newBuilder() @@ -118,20 +122,24 @@ public static void main(String[] args) { return; } - final String baseRef = arguments.baseRef; - final Path repositoryRoot = Paths.get(arguments.repositoryRoot); - final Path outputPath = arguments.outputPath != null ? Paths.get(arguments.outputPath) : null; try { - final var analyzer = new MetricsDiffAnalyzer(baseRef, GitMetricsFileFinder.getHeadReference(repositoryRoot), repositoryRoot, arguments.urlBase); + final Path repositoryRoot = arguments.repositoryRoot == null ? Paths.get(".") : Paths.get(arguments.repositoryRoot); + final String baseRef = GitMetricsFileFinder.getCommitHash(repositoryRoot, arguments.baseRef); + final String headRef = GitMetricsFileFinder.getCommitHash(repositoryRoot, arguments.headRef); + final Path outputPath = arguments.outputPath != null ? Paths.get(arguments.outputPath) : null; + + final var analyzer = new MetricsDiffAnalyzer(baseRef, headRef, repositoryRoot, arguments.urlBase); final var analysis = analyzer.analyze(); final var report = analysis.generateReport(); if (outputPath != null) { Files.writeString(outputPath, report); - logger.info(KeyValueLogMessage.of( - "Wrote metrics diff report", - "output", outputPath)); + if (logger.isInfoEnabled()) { + logger.info(KeyValueLogMessage.of( + "Wrote metrics diff report", + "output", outputPath)); + } } else { System.out.println(report); } @@ -141,8 +149,10 @@ public static void main(String[] args) { if (!outlierReport.isEmpty()) { final Path outlierQueryPath = Paths.get(arguments.outlierQueryPath); Files.writeString(outlierQueryPath, outlierReport); - logger.info(KeyValueLogMessage.of("Wrote outlier report", - "output", outlierQueryPath)); + if (logger.isInfoEnabled()) { + logger.info(KeyValueLogMessage.of("Wrote outlier report", + "output", outlierQueryPath)); + } } } } catch (final Exception e) { @@ -156,19 +166,23 @@ public static void main(String[] args) { * Performs the metrics diff analysis. * * @return analysis results - * * @throws RelationalException if analysis fails */ @Nonnull public MetricsAnalysisResult analyze() throws RelationalException { - logger.info(KeyValueLogMessage.of("Starting metrics diff analysis", - "base", baseRef)); + if (logger.isInfoEnabled()) { + logger.info(KeyValueLogMessage.of("Starting metrics diff analysis", + "base", baseRef)); + } // Find all changed metrics files - final var changedFiles = GitMetricsFileFinder.findChangedMetricsYamlFiles(baseRef, repositoryRoot); - logger.info(KeyValueLogMessage.of("Found changed metrics files", - "base", baseRef, - "file_count", changedFiles.size())); + final var changedFiles = GitMetricsFileFinder.findChangedMetricsYamlFiles(baseRef, headRef, repositoryRoot); + if (logger.isInfoEnabled()) { + logger.info(KeyValueLogMessage.of("Found changed metrics files", + "base", baseRef, + "head", headRef, + "file_count", changedFiles.size())); + } final var analysisBuilder = newAnalysisBuilder(); @@ -195,70 +209,56 @@ private void analyzeYamlFile(@Nonnull final Path yamlPath, } // Load base (old) metrics - Map baseMetrics; - try { - final var baseYamlFile = GitMetricsFileFinder.getFileAtReference( - repositoryRoot.relativize(yamlPath).toString(), baseRef, repositoryRoot); - baseMetrics = YamlExecutionContext.loadMetricsFromYamlFile(baseYamlFile); - Files.deleteIfExists(baseYamlFile); // Clean up temp file - } catch (final RelationalException e) { - // File might not exist in base ref (new file) - if (logger.isDebugEnabled()) { - logger.debug(KeyValueLogMessage.of("Could not load base metrics", - "path", yamlPath), e); - } - baseMetrics = ImmutableMap.of(); - } catch (IOException e) { - throw new RelationalException("unable to delete temporary file", ErrorCode.INTERNAL_ERROR, e); - } + final Map baseMetrics = loadMetricsAtRef(yamlPath, baseRef); // Load head (new) metrics - final Map headMetrics; - if (GitMetricsFileFinder.fileExists(yamlPath)) { - headMetrics = YamlExecutionContext.loadMetricsFromYamlFile(yamlPath); - } else { - // File was deleted - headMetrics = ImmutableMap.of(); - } + final Map headMetrics = loadMetricsAtRef(yamlPath, headRef); // Compare the metrics compareMetrics(baseMetrics, headMetrics, yamlPath, analysisBuilder); } + @Nonnull + private Map loadMetricsAtRef(@Nonnull Path yamlPath, @Nonnull String ref) throws RelationalException { + try { + final var yamlFile = GitMetricsFileFinder.getFileAtReference( + repositoryRoot.relativize(yamlPath).toString(), ref, repositoryRoot); + if (yamlFile == null) { + return ImmutableMap.of(); + } + try { + return YamlExecutionContext.loadMetricsFromYamlFile(yamlFile); + } finally { + Files.deleteIfExists(yamlFile); // Clean up temp file + } + } catch (IOException e) { + throw new RelationalException("unable to delete temporary file", ErrorCode.INTERNAL_ERROR, e); + } + } + /** * Compares metrics between base and head versions. */ @VisibleForTesting public void compareMetrics(@Nonnull final Map baseMetrics, - @Nonnull final Map headMetrics, - @Nonnull final Path filePath, - @Nonnull final MetricsAnalysisResult.Builder analysisBuilder) { - - final var baseIdentifiers = baseMetrics.keySet(); - final var headIdentifiers = headMetrics.keySet(); + @Nonnull final Map headMetrics, + @Nonnull final Path filePath, + @Nonnull final MetricsAnalysisResult.Builder analysisBuilder) { - // Find new queries (in head but not in base) + // Find new and updated queries int newCount = 0; - for (final var identifier : headIdentifiers) { - if (!baseIdentifiers.contains(identifier)) { - newCount++; - analysisBuilder.addNewQuery(filePath, identifier, headMetrics.get(identifier)); - } - } - - // Find dropped queries (in base but not in head) - int droppedCount = 0; - for (final var identifier : baseIdentifiers) { - if (!headIdentifiers.contains(identifier)) { - droppedCount++; - analysisBuilder.addDroppedQuery(filePath, identifier, baseMetrics.get(identifier)); - } - } - - // Find changed queries (in both base and head) int changedCount = 0; - for (final var identifier : headIdentifiers) { - if (baseIdentifiers.contains(identifier)) { + for (final Map.Entry entry : headMetrics.entrySet()) { + var identifier = entry.getKey(); + final MetricsInfo headMetricInfo = entry.getValue(); + final MetricsInfo baseMetricInfo = baseMetrics.get(identifier); + if (baseMetricInfo == null) { + // Query only in new metrics. Mark as new + newCount++; + analysisBuilder.addNewQuery(filePath, identifier, headMetricInfo); + } else { + // Query in both old and new. Mark as changed + changedCount++; final MetricsInfo baseInfo = baseMetrics.get(identifier); final MetricsInfo headInfo = headMetrics.get(identifier); @@ -280,6 +280,17 @@ public void compareMetrics(@Nonnull final Map entry : baseMetrics.entrySet()) { + var identifier = entry.getKey(); + final MetricsInfo baseMetricsInfo = entry.getValue(); + if (!headMetrics.containsKey(identifier)) { + droppedCount++; + analysisBuilder.addDroppedQuery(filePath, identifier, baseMetricsInfo); + } + } + if (logger.isDebugEnabled()) { logger.debug(KeyValueLogMessage.of("Analyzed metrics file", "path", filePath, @@ -306,7 +317,7 @@ public static class MetricsAnalysisResult { private final String baseRef; @Nonnull private final String headRef; - @Nullable + @Nonnull private final Path repositoryRoot; @Nullable private final String urlBase; @@ -317,7 +328,7 @@ private MetricsAnalysisResult(@Nonnull final List newQueries, @Nonnull final List metricsOnlyChanged, @Nonnull final String baseRef, @Nonnull final String headRef, - @Nullable final Path repositoryRoot, + @Nonnull final Path repositoryRoot, @Nullable final String urlBase) { this.newQueries = newQueries; this.droppedQueries = droppedQueries; @@ -347,7 +358,7 @@ public List getMetricsOnlyChanged() { @Nonnull private Path relativePath(@Nonnull Path path) { - return repositoryRoot == null ? path : repositoryRoot.relativize(path); + return repositoryRoot.relativize(path); } @Nonnull @@ -395,12 +406,23 @@ public String generateReport() { report.append("# 📊 Metrics Diff Analysis Report\n\n"); report.append("## Summary\n\n") - .append(String.format("- New queries: %d\n", newQueries.size())) - .append(String.format("- Dropped queries: %d\n", droppedQueries.size())) - .append(String.format("- Plan changed + metrics changed: %d\n", planAndMetricsChanged.size())) - .append(String.format("- Plan unchanged + metrics changed: %d\n", metricsOnlyChanged.size())) + .append("- New queries: ").append(newQueries.size()).append("\n") + .append("- Dropped queries: ").append(droppedQueries.size()).append("\n") + .append("- Plan changed + metrics changed: ").append(planAndMetricsChanged.size()).append("\n") + .append("- Plan unchanged + metrics changed: ").append(metricsOnlyChanged.size()).append("\n") .append("\n"); + report.append("
\n\n") + .append("ℹī¸ About this analysis\n\n") + .append("This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:\n\n") + .append(" - **New queries**: Queries added in this PR\n") + .append(" - **Dropped queries**: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.\n") + .append(" - **Plan changed + metrics changed**: The query plan has changed along with planner metrics.\n") + .append(" - **Metrics only changed**: Same plan but different metrics\n\n") + .append("The last category in particular may indicate planner regressions that should be investigated.\n\n") + .append("
\n\n"); + + if (!newQueries.isEmpty()) { // Only list a count of new queries by file type. Having more test cases is generally a good thing, // so we only want to get a rough overview @@ -423,24 +445,32 @@ public String generateReport() { // List out each individual dropped query. These represent a potential lack of coverage, and // so we want these listed out more explicitly report.append("## Dropped Queries\n\nThe following queries with metrics were removed:\n\n"); - for (final var change : droppedQueries) { - report.append(String.format("- %s\n", formatQueryDisplay(change))); + final int maxQueries = 20; + final List changes = droppedQueries.size() < maxQueries ? droppedQueries : droppedQueries.subList(0, maxQueries); + for (final var change : changes) { + report.append("- ").append(formatQueryDisplay(change)).append("\n"); } report.append("\n"); - } - appendChangesList(report, planAndMetricsChanged, "Plan and Metrics Changed"); - appendChangesList(report, metricsOnlyChanged, "Only Metrics Changed"); + if (droppedQueries.size() > maxQueries) { + int remaining = droppedQueries.size() - maxQueries; + report.append("This list has been truncated. There ") + .append(remaining == 1 ? "is " : "are ") + .append(remaining) + .append(" remaining dropped queries.\n\n"); + } - report.append("
\n\n") - .append("ℹī¸ About this analysis\n\n") - .append("This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:\n\n") - .append(" - **New queries**: Queries added in this PR\n") - .append(" - **Dropped queries**: Queries removed in this PR\n") - .append(" - **Plan changed + metrics changed**: The query plan has changed along with planner metrics\n") - .append(" - **Metrics only changed**: Same plan but different metrics\n\n") - .append("The last category in particular may indicate planner regressions that should be investigated.\n\n") - .append("
\n"); + report.append("The reviewer should double check that these queries were removed intentionally to avoid a loss of coverage.\n\n"); + } + + appendChangesList(report, planAndMetricsChanged, "Plan and Metrics Changed", + "These queries experienced both plan and metrics changes. This generally indicates that there was some planner change\n" + + "that means the planning for this query may be substantially different. Some amount of query plan metrics change is expected,\n" + + "but the reviewer should still validate that these changes are not excessive.\n"); + appendChangesList(report, metricsOnlyChanged, "Only Metrics Changed", + "These queries experienced *only* metrics changes without any plan changes. If these metrics have substantially changed,\n" + + "then a planner change has been made which affects planner performance but does not correlate with any new outcomes,\n" + + "which could indicate a regression.\n"); return report.toString(); } @@ -489,25 +519,26 @@ private void appendStatisticalSummary(@Nonnull final StringBuilder report, @Nonn final var fieldStats = stats.getFieldStatistics(fieldName); final var absoluteFieldStats = stats.getAbsoluteFieldStatistics(fieldName); if (fieldStats.hasChanges() || absoluteFieldStats.hasChanges()) { - report.append(String.format("**`%s`**:\n", fieldName)); - report.append(String.format(" - Average change: %s%.1f\n", sign(fieldStats.getMean()), fieldStats.getMean())); - report.append(String.format(" - Average absolute change: %s%.1f\n", sign(absoluteFieldStats.getMean()), absoluteFieldStats.getMean())); - report.append(String.format(" - Median change: %s%d\n", sign(fieldStats.getMedian()), fieldStats.getMedian())); - report.append(String.format(" - Median absolute change: %s%d\n", sign(absoluteFieldStats.getMedian()), absoluteFieldStats.getMedian())); - report.append(String.format(" - Standard deviation: %.1f\n", fieldStats.getStandardDeviation())); - report.append(String.format(" - Standard absolute deviation: %.1f\n", absoluteFieldStats.getStandardDeviation())); - report.append(String.format(" - Range: %s%d to %s%d\n", sign(fieldStats.getMin()), fieldStats.getMin(), sign(fieldStats.getMax()), fieldStats.getMax())); - report.append(String.format(" - Range of absolute values: %s%d to %s%d\n", sign(absoluteFieldStats.getMin()), absoluteFieldStats.getMin(), sign(absoluteFieldStats.getMax()), absoluteFieldStats.getMax())); - report.append(String.format(" - Queries affected: %d\n\n", fieldStats.getChangedCount())); + report.append(String.format("**`%s`**:%n", fieldName)); + report.append(String.format(" - Average change: %s%.1f%n", sign(fieldStats.getMean()), fieldStats.getMean())); + report.append(String.format(" - Average absolute change: %s%.1f%n", sign(absoluteFieldStats.getMean()), absoluteFieldStats.getMean())); + report.append(String.format(" - Median change: %s%d%n", sign(fieldStats.getMedian()), fieldStats.getMedian())); + report.append(String.format(" - Median absolute change: %s%d%n", sign(absoluteFieldStats.getMedian()), absoluteFieldStats.getMedian())); + report.append(String.format(" - Standard deviation: %.1f%n", fieldStats.getStandardDeviation())); + report.append(String.format(" - Standard absolute deviation: %.1f%n", absoluteFieldStats.getStandardDeviation())); + report.append(String.format(" - Range: %s%d to %s%d%n", sign(fieldStats.getMin()), fieldStats.getMin(), sign(fieldStats.getMax()), fieldStats.getMax())); + report.append(String.format(" - Range of absolute values: %s%d to %s%d%n", sign(absoluteFieldStats.getMin()), absoluteFieldStats.getMin(), sign(absoluteFieldStats.getMax()), absoluteFieldStats.getMax())); + report.append(String.format(" - Queries affected: %d%n%n", fieldStats.getChangedCount())); } } } - private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull List changes, @Nonnull String title) { + private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull List changes, @Nonnull String title, @Nonnull String explanation) { if (changes.isEmpty()) { return; } report.append("## ").append(title).append("\n\n"); + report.append(explanation).append("\n"); report.append("Total: ").append(changes.size()).append(" quer").append(changes.size() == 1 ? "y" : "ies").append("\n\n"); // Statistical analysis @@ -524,6 +555,14 @@ private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull Lis for (final var change : metricsOutliers) { report.append(String.format("- %s", formatQueryDisplay(change))).append("\n"); if (change.oldInfo != null && change.newInfo != null) { + final String oldExplain = change.oldInfo.getExplain(); + final String newExplain = change.newInfo.getExplain(); + if (oldExplain.equals(newExplain)) { + report.append(" - explain: `").append(oldExplain).append("`\n"); + } else { + report.append(" - old explain: `").append(oldExplain).append("`\n"); + report.append(" - new explain: `").append(newExplain).append("`\n"); + } appendMetricsDiff(report, change); } } @@ -551,8 +590,8 @@ private MetricsStatistics calculateMetricsStatistics(@Nonnull final List findOutliers(@Nonnull final List changes, boolean isOutlier = false; for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { final var field = descriptor.findFieldByName(fieldName); - final var oldValue = (long) oldMetrics.getField(field); - final var newValue = (long) newMetrics.getField(field); + final var oldValue = (long)oldMetrics.getField(field); + final var newValue = (long)newMetrics.getField(field); if (oldValue != newValue) { final var difference = newValue - oldValue; @@ -642,8 +681,9 @@ private void appendMetricsDiff(@Nonnull final StringBuilder report, final long newValue = (long)newMetrics.getField(field); if (oldValue != newValue) { final long change = newValue - oldValue; - final String changeStr = sign(change) + change; - report.append(String.format(" - `%s`: %d -> %d (%s)\n", fieldName, oldValue, newValue, changeStr)); + report.append(" - `").append(fieldName).append("`: ") + .append(oldValue).append(" -> ").append(newValue) + .append(" (").append(sign(change)).append(change).append(")\n"); } } } @@ -657,48 +697,53 @@ public static class Builder { private final String baseRef; @Nonnull private final String headRef; - @Nullable + @Nonnull private final Path repositoryRoot; @Nullable private final String urlBase; - public Builder(@Nonnull String baseRef, @Nonnull String headRef, @Nullable final Path repositoryRoot, @Nullable final String urlBase) { + public Builder(@Nonnull String baseRef, @Nonnull String headRef, @Nonnull final Path repositoryRoot, @Nullable final String urlBase) { this.baseRef = baseRef; this.headRef = headRef; this.repositoryRoot = repositoryRoot; this.urlBase = urlBase; } + @Nonnull public Builder addNewQuery(@Nonnull final Path filePath, - @Nonnull final PlannerMetricsProto.Identifier identifier, - @Nonnull final MetricsInfo info) { + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo info) { newQueries.add(new QueryChange(filePath, identifier, null, info)); return this; } + @Nonnull public Builder addDroppedQuery(@Nonnull final Path filePath, - @Nonnull final PlannerMetricsProto.Identifier identifier, - @Nonnull final MetricsInfo info) { + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo info) { droppedQueries.add(new QueryChange(filePath, identifier, info, null)); return this; } + @Nonnull public Builder addPlanAndMetricsChanged(@Nonnull final Path filePath, - @Nonnull final PlannerMetricsProto.Identifier identifier, - @Nonnull final MetricsInfo oldInfo, - @Nonnull final MetricsInfo newInfo) { + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo oldInfo, + @Nonnull final MetricsInfo newInfo) { planAndMetricsChanged.add(new QueryChange(filePath, identifier, oldInfo, newInfo)); return this; } + @Nonnull public Builder addMetricsOnlyChanged(@Nonnull final Path filePath, - @Nonnull final PlannerMetricsProto.Identifier identifier, - @Nonnull final MetricsInfo oldInfo, - @Nonnull final MetricsInfo newInfo) { + @Nonnull final PlannerMetricsProto.Identifier identifier, + @Nonnull final MetricsInfo oldInfo, + @Nonnull final MetricsInfo newInfo) { metricsOnlyChanged.add(new QueryChange(filePath, identifier, oldInfo, newInfo)); return this; } + @Nonnull public MetricsAnalysisResult build() { return new MetricsAnalysisResult( newQueries.build(), diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java index c04f5841fa..b8b41b6460 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java @@ -28,7 +28,7 @@ public final class MetricsInfo { @Nonnull - private final PlannerMetricsProto.Info metricsInfo; + private final PlannerMetricsProto.Info underlying; @Nonnull private final Path filePath; private final int lineNumber; @@ -36,14 +36,14 @@ public final class MetricsInfo { MetricsInfo(@Nonnull PlannerMetricsProto.Info metricsInfo, @Nonnull Path filePath, int lineNumber) { - this.metricsInfo = metricsInfo; + this.underlying = metricsInfo; this.filePath = filePath; this.lineNumber = lineNumber; } @Nonnull - public PlannerMetricsProto.Info getMetricsInfo() { - return metricsInfo; + public PlannerMetricsProto.Info getUnderlying() { + return underlying; } @Nonnull @@ -57,12 +57,12 @@ public int getLineNumber() { @Nonnull public String getExplain() { - return metricsInfo.getExplain(); + return underlying.getExplain(); } @Nonnull public PlannerMetricsProto.CountersAndTimers getCountersAndTimers() { - return metricsInfo.getCountersAndTimers(); + return underlying.getCountersAndTimers(); } @Override @@ -74,11 +74,11 @@ public boolean equals(final Object object) { return false; } final MetricsInfo that = (MetricsInfo)object; - return lineNumber == that.lineNumber && Objects.equals(metricsInfo, that.metricsInfo) && Objects.equals(filePath, that.filePath); + return lineNumber == that.lineNumber && Objects.equals(underlying, that.underlying) && Objects.equals(filePath, that.filePath); } @Override public int hashCode() { - return Objects.hash(metricsInfo, filePath, lineNumber); + return Objects.hash(underlying, filePath, lineNumber); } } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java index dc43e21302..e3df335fe7 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java @@ -518,7 +518,7 @@ private static ImmutableMap info + * @return immutable map of identifier to info * @throws RelationalException if file cannot be read or parsed */ @Nonnull @@ -567,10 +567,10 @@ public static Map loadMetricsFromYa if (key instanceof CustomYamlConstructor.LinedObject) { CustomYamlConstructor.LinedObject linedObject = (CustomYamlConstructor.LinedObject) key; final String keyString = (String) linedObject.getObject(); - if (keyString.equals("query")) { + if ("query".equals(keyString)) { query = (String) entry.getValue(); lineNumber = ((CustomYamlConstructor.LinedObject) key).getLineNumber(); - } else if (keyString.equals("explain")) { + } else if ("explain".equals(keyString)) { explain = (String) entry.getValue(); } } @@ -626,7 +626,7 @@ private static void processQueryAtLine(Map queryMap, // Build info final var info = PlannerMetricsProto.Info.newBuilder() - .setExplain(explain) + .setExplain(explain == null ? "" : explain) .setCountersAndTimers(countersAndTimers) .build(); diff --git a/yaml-tests/yaml-tests.gradle b/yaml-tests/yaml-tests.gradle index 8b4567ff28..84c9ad2048 100644 --- a/yaml-tests/yaml-tests.gradle +++ b/yaml-tests/yaml-tests.gradle @@ -248,6 +248,9 @@ tasks.register('analyzeMetrics', JavaExec) { // args += ['--base-ref', project.getProperty('metricsAnalysis.baseRef')] args += ['--base-ref', '4.5.10.0'] } + if (project.hasProperty('metricsAnalysis.headRef')) { + args += ['--head-ref', project.getProperty('metricsAnalysis.headRef')] + } if (project.hasProperty('metricsAnalysis.urlBase')) { args += ['--url-base', project.getProperty('metricsAnalysis.urlBase')] } From 0eb8464a82ddf09b56e78e4a6e0455aeb9db95bd Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 19:16:14 +0100 Subject: [PATCH 16/24] use better head and base sha spec --- .github/workflows/metrics_analysis.yml | 5 ++- .../yamltests/GitMetricsFileFinder.java | 25 +++++-------- .../yamltests/MetricsDiffAnalyzer.java | 3 +- .../relational/yamltests/MetricsInfo.java | 36 +++++++++++++++++++ .../yamltests/YamlExecutionContext.java | 35 ------------------ .../queryconfigs/CheckExplainConfig.java | 11 ++++-- 6 files changed, 56 insertions(+), 59 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index ce32e54f61..51ef57a361 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -2,7 +2,6 @@ name: Metrics Diff Analysis on: pull_request: - branches: [ main ] paths: - '**/*.metrics.yaml' - '**/*.metrics.binpb' @@ -30,8 +29,8 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.base_ref }}" \ - -PmetricsAnalysis.headRef="${{ github.head_ref }}" \ + -PmetricsAnalysis.baseRef="${{ github.pull_request.base.sha }}" \ + -PmetricsAnalysis.headRef="${{ github.pull_request.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java index c951c0e25d..29a6232930 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java @@ -56,7 +56,7 @@ private GitMetricsFileFinder() { * @param baseRef the base git reference (e.g., "main", "HEAD~1", commit SHA) * @param headRef the target git reference (e.g., "main", "HEAD~1", commit SHA) * @param repositoryRoot the root directory of the git repository - * @return set of paths to changed metrics files (both .yaml and .binpb) + * @return set of paths to changed metrics YAML files * @throws RelationalException if git command fails or repository is not valid */ @Nonnull @@ -125,10 +125,9 @@ private static List getChangedFiles(@Nonnull final String baseRef, } /** - * Checks if a file path represents a metrics file (either .yaml or .binpb). + * Checks if a file path represents a metrics YAML file. * * @param filePath the file path to check - * * @return true if the file is a metrics file */ private static boolean isMetricsYamlFile(@Nonnull final String filePath) { @@ -136,23 +135,17 @@ private static boolean isMetricsYamlFile(@Nonnull final String filePath) { } /** - * Checks if a file exists at the specified path. - * - * @param filePath the path to check - * @return true if the file exists - */ - public static boolean fileExists(@Nonnull final Path filePath) { - return Files.exists(filePath) && Files.isRegularFile(filePath); - } - - /** - * Gets the file path for a specific git reference (commit). - * This method checks out the file content at the specified reference. + * Gets the file contents for a specific git reference (commit). + * This method checks out the file content at the specified reference and + * saves it into a temporary file located at the returned {@link Path}. + * If the file does not exist at the given reference, it will return a + * {@code null} path. * * @param filePath the relative file path * @param gitRef the git reference * @param repositoryRoot the repository root - * @return path to a temporary file containing the content at the specified reference or {@code null} if it didn't exist + * @return path to a temporary file containing the content at the specified reference + * or {@code null} if it didn't exist at the given ref * @throws RelationalException if git command fails */ @Nullable diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index c3af084359..98a9590b5a 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -263,8 +263,7 @@ public void compareMetrics(@Nonnull final Map isMetricDifferent(expected, actual, field)); + } + + /** + * Compares a specific metric field between expected and actual values. + * + * @param expected the expected metrics + * @param actual the actual metrics + * @param fieldDescriptor the field to compare + * @return true if the metric values differ + */ + private static boolean isMetricDifferent(@Nonnull final MetricsInfo expected, + @Nonnull final MetricsInfo actual, + @Nonnull final Descriptors.FieldDescriptor fieldDescriptor) { + final long expectedMetric = (long) expected.getUnderlying().getField(fieldDescriptor); + final long actualMetric = (long) actual.getUnderlying().getField(fieldDescriptor); + return expectedMetric != actualMetric; + } } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java index e3df335fe7..15806c605e 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java @@ -30,7 +30,6 @@ import com.google.common.base.Verify; import com.google.common.collect.ImmutableMap; import com.google.common.collect.LinkedListMultimap; -import com.google.protobuf.Descriptors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.junit.jupiter.api.Assertions; @@ -655,40 +654,6 @@ private static long getLongValue(Map map, String key) { throw new IllegalArgumentException("Expected numeric value for key: " + key + ", got: " + value); } - /** - * Compares two CountersAndTimers and determines if any of the tracked metrics are different. - * This method checks the core metrics that are used for planner comparison but excludes timing - * information as those can vary between runs. - * - * @param expected the expected metrics values - * @param actual the actual metrics values - * @return true if any of the tracked metrics differ - */ - public static boolean areMetricsDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, - @Nonnull final PlannerMetricsProto.CountersAndTimers actual) { - final var metricsDescriptor = expected.getDescriptorForType(); - - return TRACKED_METRIC_FIELDS.stream() - .map(metricsDescriptor::findFieldByName) - .anyMatch(field -> isMetricDifferent(expected, actual, field)); - } - - /** - * Compares a specific metric field between expected and actual values. - * - * @param expected the expected metrics - * @param actual the actual metrics - * @param fieldDescriptor the field to compare - * @return true if the metric values differ - */ - public static boolean isMetricDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, - @Nonnull final PlannerMetricsProto.CountersAndTimers actual, - @Nonnull final Descriptors.FieldDescriptor fieldDescriptor) { - final long expectedMetric = (long) expected.getField(fieldDescriptor); - final long actualMetric = (long) actual.getField(fieldDescriptor); - return expectedMetric != actualMetric; - } - @Nonnull private static String metricsBinaryProtoFileName(@Nonnull final String resourcePath) { return baseName(resourcePath) + ".metrics.binpb"; diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java index 3fa5bff9a1..1ae90a1dad 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/queryconfigs/CheckExplainConfig.java @@ -206,9 +206,14 @@ private void checkMetrics(final @Nonnull String currentQuery, private boolean areMetricsDifferent(final PlannerMetricsProto.CountersAndTimers expectedCountersAndTimers, final PlannerMetricsProto.CountersAndTimers actualCountersAndTimers, final Descriptors.Descriptor metricsDescriptor) { - return YamlExecutionContext.TRACKED_METRIC_FIELDS.stream() - .map(metricsDescriptor::findFieldByName) - .anyMatch(field -> isMetricDifferent(expectedCountersAndTimers, actualCountersAndTimers, field, lineNumber)); + boolean different = false; + for (String fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + // Check each metric. Do NOT short-circuit because we want to log any metrics + // that have changed (a side effect of isMetricDifferent) + different |= isMetricDifferent(expectedCountersAndTimers, actualCountersAndTimers, + metricsDescriptor.findFieldByName(fieldName), lineNumber); + } + return different; } private static boolean isMetricDifferent(@Nonnull final PlannerMetricsProto.CountersAndTimers expected, From bdd1f1ef91b424e221611efab0ca20341d8ef1c7 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 19:26:27 +0100 Subject: [PATCH 17/24] fix name of reference to commit hashes --- .github/workflows/metrics_analysis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 51ef57a361..9150049724 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -29,8 +29,8 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.pull_request.base.sha }}" \ - -PmetricsAnalysis.headRef="${{ github.pull_request.head.sha }}" \ + -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ + -PmetricsAnalysis.headRef="${{ github.event.pull_request.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ From 68da8ad9aeba05c5a869d613e5879d14021374a3 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 19:30:31 +0100 Subject: [PATCH 18/24] doh get the underlying counters when comparing metrics --- .../apple/foundationdb/relational/yamltests/MetricsInfo.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java index aaf207cc33..e430b59918 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsInfo.java @@ -113,8 +113,8 @@ public static boolean areMetricsDifferent(@Nonnull final MetricsInfo expected, private static boolean isMetricDifferent(@Nonnull final MetricsInfo expected, @Nonnull final MetricsInfo actual, @Nonnull final Descriptors.FieldDescriptor fieldDescriptor) { - final long expectedMetric = (long) expected.getUnderlying().getField(fieldDescriptor); - final long actualMetric = (long) actual.getUnderlying().getField(fieldDescriptor); + final long expectedMetric = (long) expected.getUnderlying().getCountersAndTimers().getField(fieldDescriptor); + final long actualMetric = (long) actual.getUnderlying().getCountersAndTimers().getField(fieldDescriptor); return expectedMetric != actualMetric; } } From 13c5d8649a4ab664e38a3e861048e3b8dcefc51d Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 19:48:37 +0100 Subject: [PATCH 19/24] fix some teamscale findings --- .../yamltests/MetricsDiffAnalyzer.java | 47 ++++++++------- .../yamltests/YamlExecutionContext.java | 57 +++++++++---------- .../yamltests/MetricsDiffAnalyzerTest.java | 21 ------- 3 files changed, 53 insertions(+), 72 deletions(-) diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index 98a9590b5a..ddb8bf74d2 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -611,11 +611,13 @@ public List findAllOutliers() { .build(); } + @Nonnull private List findOutliers(@Nonnull final List changes) { final MetricsStatistics summary = calculateMetricsStatistics(changes); return findOutliers(changes, summary); } + @Nonnull private List findOutliers(@Nonnull final List changes, @Nonnull final MetricsStatistics stats) { if (changes.size() < 3) { // Not enough data for meaningful outlier detection @@ -625,34 +627,37 @@ private List findOutliers(@Nonnull final List changes, final ImmutableList.Builder outliers = ImmutableList.builder(); for (final var change : changes) { - if (change.oldInfo != null && change.newInfo != null) { - final var oldMetrics = change.oldInfo.getCountersAndTimers(); - final var newMetrics = change.newInfo.getCountersAndTimers(); - final var descriptor = oldMetrics.getDescriptorForType(); + if (isOutlier(change, stats)) { + outliers.add(change); + } + } - boolean isOutlier = false; - for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { - final var field = descriptor.findFieldByName(fieldName); - final var oldValue = (long)oldMetrics.getField(field); - final var newValue = (long)newMetrics.getField(field); + return outliers.build(); + } - if (oldValue != newValue) { - final var difference = newValue - oldValue; - if (isOutlierValue(stats.getFieldStatistics(fieldName), difference) - || isOutlierValue(stats.getAbsoluteFieldStatistics(fieldName), Math.abs(difference))) { - isOutlier = true; - break; - } - } - } + private boolean isOutlier(@Nonnull QueryChange change, @Nonnull MetricsStatistics stats) { + if (change.oldInfo == null || change.newInfo == null) { + return false; + } + final var oldMetrics = change.oldInfo.getCountersAndTimers(); + final var newMetrics = change.newInfo.getCountersAndTimers(); + final var descriptor = oldMetrics.getDescriptorForType(); + + for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { + final var field = descriptor.findFieldByName(fieldName); + final var oldValue = (long)oldMetrics.getField(field); + final var newValue = (long)newMetrics.getField(field); - if (isOutlier) { - outliers.add(change); + if (oldValue != newValue) { + final var difference = newValue - oldValue; + if (isOutlierValue(stats.getFieldStatistics(fieldName), difference) + || isOutlierValue(stats.getAbsoluteFieldStatistics(fieldName), Math.abs(difference))) { + return true; } } } - return outliers.build(); + return false; } private boolean isOutlierValue(MetricsStatistics.FieldStatistics fieldStats, long difference) { diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java index 15806c605e..3d63d9e65b 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java @@ -552,32 +552,7 @@ public static Map loadMetricsFromYa // Process each query in the block for (final var queryMap : queries) { - if (queryMap == null) { - continue; - } - - // Extract line number from the "query" key if it's a LinedObject - int lineNumber = 1; // Default line number - String query = null; - String explain = null; - - for (final var entry : queryMap.entrySet()) { - final Object key = entry.getKey(); - if (key instanceof CustomYamlConstructor.LinedObject) { - CustomYamlConstructor.LinedObject linedObject = (CustomYamlConstructor.LinedObject) key; - final String keyString = (String) linedObject.getObject(); - if ("query".equals(keyString)) { - query = (String) entry.getValue(); - lineNumber = ((CustomYamlConstructor.LinedObject) key).getLineNumber(); - } else if ("explain".equals(keyString)) { - explain = (String) entry.getValue(); - } - } - } - - if (query != null) { - processQueryAtLine(queryMap, blockName, lineNumber, query, explain, seen, resultMapBuilder, filePath); - } + processQueryAtLine(queryMap, blockName, seen, resultMapBuilder, filePath); } } @@ -591,14 +566,36 @@ public static Map loadMetricsFromYa * Processes a single query with its line number information. */ @SuppressWarnings("unchecked") - private static void processQueryAtLine(Map queryMap, + private static void processQueryAtLine(@Nullable Map queryMap, String blockName, - int lineNumber, - String query, - @Nullable String explain, Map seen, ImmutableMap.Builder resultMapBuilder, Path filePath) { + if (queryMap == null) { + return; + } + + String query = null; + String explain = null; + int lineNumber = 1; // Default line number + for (Map.Entry entry : queryMap.entrySet()) { + final Object key = entry.getKey(); + if (key instanceof CustomYamlConstructor.LinedObject) { + CustomYamlConstructor.LinedObject linedObject = (CustomYamlConstructor.LinedObject) key; + final String keyString = (String) linedObject.getObject(); + if ("query".equals(keyString)) { + query = (String) entry.getValue(); + lineNumber = ((CustomYamlConstructor.LinedObject) key).getLineNumber(); + } else if ("explain".equals(keyString)) { + explain = (String) entry.getValue(); + } + } + } + if (query == null) { + // Query not found + return; + } + // Extract the query string, handling LinedObject if present final var setup = (List) queryMap.get("setup"); diff --git a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java index b90d18da77..670cfbb5b1 100644 --- a/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java +++ b/yaml-tests/src/test/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzerTest.java @@ -362,25 +362,4 @@ private Map createMetricsWithOutlie return builder.build(); } - - @Nonnull - private MetricsDiffAnalyzer.QueryChange createDummyQueryChange() { - final var identifier = PlannerMetricsProto.Identifier.newBuilder() - .setBlockName("test_block") - .setQuery("dummy_query") - .build(); - final var info = PlannerMetricsProto.Info.newBuilder() - .setExplain("dummy_explain") - .setCountersAndTimers(BASE_DUMMY_COUNTERS) - .build(); - - final var metricsInfo = new MetricsInfo(info, Paths.get("test.yaml"), 1); - - return new MetricsDiffAnalyzer.QueryChange( - Paths.get("test.metrics.yaml"), - identifier, - metricsInfo, - metricsInfo - ); - } } From e5cc13c681639d002e81274ddad6b10b0ba020fb Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 23 Sep 2025 19:50:47 +0100 Subject: [PATCH 20/24] use pull_request_target instead of pull_request to allow commenting --- .github/workflows/metrics_analysis.yml | 8 +++----- yaml-tests/yaml-tests.gradle | 3 +-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 9150049724..c210b817b9 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -1,12 +1,10 @@ name: Metrics Diff Analysis on: - pull_request: + pull_request_target: paths: - '**/*.metrics.yaml' - '**/*.metrics.binpb' - - 'yaml-tests/**' - - '.github/workflows/metrics_analysis.yml' jobs: metrics-analysis: @@ -29,8 +27,8 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.event.pull_request.base.sha }}" \ - -PmetricsAnalysis.headRef="${{ github.event.pull_request.head.sha }}" \ + -PmetricsAnalysis.baseRef="${{ github.event.pull_request_target.base.sha }}" \ + -PmetricsAnalysis.headRef="${{ github.event.pull_request_target.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ diff --git a/yaml-tests/yaml-tests.gradle b/yaml-tests/yaml-tests.gradle index 84c9ad2048..8ed4e50c92 100644 --- a/yaml-tests/yaml-tests.gradle +++ b/yaml-tests/yaml-tests.gradle @@ -245,8 +245,7 @@ tasks.register('analyzeMetrics', JavaExec) { // Pass along arguments to metrics analysis args = [] if (project.hasProperty('metricsAnalysis.baseRef')) { - // args += ['--base-ref', project.getProperty('metricsAnalysis.baseRef')] - args += ['--base-ref', '4.5.10.0'] + args += ['--base-ref', project.getProperty('metricsAnalysis.baseRef')] } if (project.hasProperty('metricsAnalysis.headRef')) { args += ['--head-ref', project.getProperty('metricsAnalysis.headRef')] From 476b8d83814ce90e2e6f40ad291e5e6605c51ec4 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 24 Sep 2025 12:45:42 +0100 Subject: [PATCH 21/24] track and report regressions rather than absolute values ; also delete rather than updating comment and use more accurate sha --- .github/workflows/metrics_analysis.yml | 28 +++---- .../yamltests/MetricsDiffAnalyzer.java | 50 +++++++---- .../yamltests/MetricsStatistics.java | 44 +++++----- .../yamltests/MetricsDiffAnalyzerTest.java | 4 +- .../yamltests/MetricsDiffIntegrationTest.java | 13 +-- .../yamltests/MetricsStatisticsTest.java | 84 +++++++++---------- 6 files changed, 119 insertions(+), 104 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index c210b817b9..1dd6e946ac 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -4,7 +4,6 @@ on: pull_request_target: paths: - '**/*.metrics.yaml' - - '**/*.metrics.binpb' jobs: metrics-analysis: @@ -27,8 +26,8 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.event.pull_request_target.base.sha }}" \ - -PmetricsAnalysis.headRef="${{ github.event.pull_request_target.head.sha }}" \ + -PmetricsAnalysis.baseRef="${{ github.sha }}" \ + -PmetricsAnalysis.headRef="${{ github.event.pull_request.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ -PmetricsAnalysis.output="${{ github.workspace }}/metrics-analysis-output.txt" \ @@ -67,23 +66,22 @@ jobs: ); if (existingComment) { - // Update existing comment - await github.rest.issues.updateComment({ + // Delete previous comment + await github.rest.issues.deleteComment({ owner: context.repo.owner, repo: context.repo.repo, comment_id: existingComment.id, - body: report - }); - } else { - // Create new comment - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: report }); } + // Create new comment + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: report + }); + - name: Add inline comments for outliers uses: actions/github-script@v7 if: steps.check-changes.outputs.SIGNIFICANT_CHANGES == 'true' @@ -137,7 +135,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, pull_number: context.issue.number, - body: "**Significant Metrics Change**\n\nThis query's metrics changed significantly during the latest run.\n\n" + data + "\n", + body: "**Significant Metrics Change**\n\nThis query's metrics have changed significantly.\n\n" + data + "\n", path: filePath, line: lineNumber, side: 'RIGHT' diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index ddb8bf74d2..2016dae0d0 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -516,18 +516,32 @@ private String sign(long l) { private void appendStatisticalSummary(@Nonnull final StringBuilder report, @Nonnull final MetricsStatistics stats) { for (final var fieldName : YamlExecutionContext.TRACKED_METRIC_FIELDS) { final var fieldStats = stats.getFieldStatistics(fieldName); - final var absoluteFieldStats = stats.getAbsoluteFieldStatistics(fieldName); - if (fieldStats.hasChanges() || absoluteFieldStats.hasChanges()) { + final var regressionFieldStats = stats.getRegressionStatistics(fieldName); + if (fieldStats.hasChanges() || regressionFieldStats.hasChanges()) { report.append(String.format("**`%s`**:%n", fieldName)); report.append(String.format(" - Average change: %s%.1f%n", sign(fieldStats.getMean()), fieldStats.getMean())); - report.append(String.format(" - Average absolute change: %s%.1f%n", sign(absoluteFieldStats.getMean()), absoluteFieldStats.getMean())); + if (regressionFieldStats.hasChanges()) { + report.append(String.format(" - Average regression: %s%.1f%n", sign(regressionFieldStats.getMean()), regressionFieldStats.getMean())); + } report.append(String.format(" - Median change: %s%d%n", sign(fieldStats.getMedian()), fieldStats.getMedian())); - report.append(String.format(" - Median absolute change: %s%d%n", sign(absoluteFieldStats.getMedian()), absoluteFieldStats.getMedian())); + if (regressionFieldStats.hasChanges()) { + report.append(String.format(" - Median regression: %s%d%n", sign(regressionFieldStats.getMedian()), regressionFieldStats.getMedian())); + } report.append(String.format(" - Standard deviation: %.1f%n", fieldStats.getStandardDeviation())); - report.append(String.format(" - Standard absolute deviation: %.1f%n", absoluteFieldStats.getStandardDeviation())); + if (regressionFieldStats.hasChanges()) { + report.append(String.format(" - Standard deviation of regressions: %.1f%n", regressionFieldStats.getStandardDeviation())); + } report.append(String.format(" - Range: %s%d to %s%d%n", sign(fieldStats.getMin()), fieldStats.getMin(), sign(fieldStats.getMax()), fieldStats.getMax())); - report.append(String.format(" - Range of absolute values: %s%d to %s%d%n", sign(absoluteFieldStats.getMin()), absoluteFieldStats.getMin(), sign(absoluteFieldStats.getMax()), absoluteFieldStats.getMax())); - report.append(String.format(" - Queries affected: %d%n%n", fieldStats.getChangedCount())); + if (regressionFieldStats.hasChanges()) { + report.append(String.format(" - Range of regressions: %s%d to %s%d%n", sign(regressionFieldStats.getMin()), regressionFieldStats.getMin(), sign(regressionFieldStats.getMax()), regressionFieldStats.getMax())); + } + report.append(String.format(" - Queries changed: %d%n", fieldStats.getChangedCount())); + if (regressionFieldStats.hasChanges()) { + report.append(String.format(" - Queries regressed: %d%n", regressionFieldStats.getChangedCount())); + } else { + report.append(" - No regressions! 🎉\n"); + } + report.append("\n"); // End with blank line } } } @@ -547,10 +561,18 @@ private void appendChangesList(@Nonnull final StringBuilder report, @Nonnull Lis // Show outliers for metrics-only changes (these are more concerning) final var metricsOutliers = findOutliers(changes, summary); - report.append("### Significant Changes (").append(title).append(")\n\n"); if (metricsOutliers.isEmpty()) { - report.append("No outliers detected.\n\n"); + report.append("There were no queries with significant regressions detected.\n\n"); } else { + report.append("### Significant Regressions (").append(title).append(")\n\n"); + report.append("There ") + .append(metricsOutliers.size() == 1 ? "was " : "were ") + .append(metricsOutliers.size()) + .append(" outlier").append(metricsOutliers.size() == 1 ? "" : "s") + .append(" detected. Outlier queries have a significant regression in at least one field. Statistically, " + + "this represents either an increase of more than two standard deviations above " + + "the mean or a large absolute increase (e.g., 100).\n\n"); + for (final var change : metricsOutliers) { report.append(String.format("- %s", formatQueryDisplay(change))).append("\n"); if (change.oldInfo != null && change.newInfo != null) { @@ -593,8 +615,7 @@ private MetricsStatistics calculateMetricsStatistics(@Nonnull final List 0 && isOutlierValue(stats.getRegressionStatistics(fieldName), difference)) { return true; } } @@ -664,8 +684,8 @@ private boolean isOutlierValue(MetricsStatistics.FieldStatistics fieldStats, lon // Consider it an outlier if it's more than 2 standard deviations from the mean // or if it's a very large absolute change Assert.thatUnchecked(fieldStats.hasChanges(), "Field stats should have at least one difference"); - final var zScore = Math.abs((difference - fieldStats.mean) / fieldStats.standardDeviation); - final var isLargeAbsoluteChange = Math.abs(difference) > Math.max(100, Math.abs(fieldStats.mean) * 2); + final var zScore = Math.abs((difference - fieldStats.getMean()) / fieldStats.getStandardDeviation()); + final var isLargeAbsoluteChange = Math.abs(difference) > Math.max(100, Math.abs(fieldStats.getMean()) * 2); return zScore > 2.0 || isLargeAbsoluteChange; } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java index 6782b3d82b..947569b114 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsStatistics.java @@ -36,12 +36,12 @@ */ public final class MetricsStatistics { private final Map fieldStatistics; - private final Map absoluteFieldStatistics; + private final Map regressionFieldStatistics; private MetricsStatistics(@Nonnull final Map fieldStatistics, - @Nonnull final Map absoluteFieldStatistics) { + @Nonnull final Map regressionFieldStatistics) { this.fieldStatistics = fieldStatistics; - this.absoluteFieldStatistics = absoluteFieldStatistics; + this.regressionFieldStatistics = regressionFieldStatistics; } @Nonnull @@ -50,21 +50,23 @@ public FieldStatistics getFieldStatistics(@Nonnull final String fieldName) { } @Nonnull - public FieldStatistics getAbsoluteFieldStatistics(@Nonnull final String fieldName) { - return absoluteFieldStatistics.getOrDefault(fieldName, FieldStatistics.EMPTY); + public FieldStatistics getRegressionStatistics(@Nonnull final String fieldName) { + return regressionFieldStatistics.getOrDefault(fieldName, FieldStatistics.EMPTY); } /** * Statistics for a single metrics field across all queries. */ public static class FieldStatistics { + @Nonnull public static final FieldStatistics EMPTY = new FieldStatistics(ImmutableList.of(), 0.0, 0.0); + @Nonnull public final List sortedValues; public final double mean; public final double standardDeviation; - private FieldStatistics(final List sortedValues, + private FieldStatistics(@Nonnull final List sortedValues, final double mean, final double standardDeviation) { this.sortedValues = ImmutableList.copyOf(sortedValues); @@ -116,14 +118,6 @@ public long getQuantile(double quantile) { public long getMedian() { return getQuantile(0.5); } - - public long getP95() { - return getQuantile(0.95); - } - - public long getP05() { - return getQuantile(0.05); - } } public static Builder newBuilder() { @@ -134,31 +128,37 @@ public static Builder newBuilder() { * Builder for collecting metric differences and calculating statistics. */ public static class Builder { + @Nonnull private final Map> differences = new HashMap<>(); - private final Map> absoluteDifferences = new HashMap<>(); + @Nonnull + private final Map> regressions = new HashMap<>(); @Nonnull - public Builder addDifference(@Nonnull final String fieldName, final long difference) { + public Builder addDifference(@Nonnull final String fieldName, final long baseValue, final long headValue) { + long difference = headValue - baseValue; differences.computeIfAbsent(fieldName, k -> new ArrayList<>()).add(difference); - absoluteDifferences.computeIfAbsent(fieldName, k -> new ArrayList<>()).add(Math.abs(difference)); + if (difference > 0) { + regressions.computeIfAbsent(fieldName, k -> new ArrayList<>()).add(difference); + } return this; } + @Nonnull private Map buildStats(@Nonnull final Map> baseMap) { final ImmutableMap.Builder builder = ImmutableMap.builder(); for (final var entry : baseMap.entrySet()) { - final var fieldName = entry.getKey(); - final var values = entry.getValue(); + final String fieldName = entry.getKey(); + final List values = entry.getValue(); if (values.isEmpty()) { continue; } - // Sort values for median calculation + // Sort values for quantile calculations Collections.sort(values); - // Calculate statistics + // Calculate statistics for values final var mean = values.stream().mapToLong(Long::longValue).average().orElse(0.0); final var variance = values.stream() .mapToDouble(v -> Math.pow(v - mean, 2)) @@ -173,7 +173,7 @@ private Map buildStats(@Nonnull final Map Date: Wed, 24 Sep 2025 12:47:27 +0100 Subject: [PATCH 22/24] use strings provided rather than finding the hashes --- .../yamltests/GitMetricsFileFinder.java | 34 ------------------- .../yamltests/MetricsDiffAnalyzer.java | 4 +-- 2 files changed, 1 insertion(+), 37 deletions(-) diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java index 29a6232930..d1c1db12a7 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/GitMetricsFileFinder.java @@ -180,38 +180,4 @@ public static Path getFileAtReference(@Nonnull final String filePath, "Failed to get file content at reference " + gitRef + ":" + filePath, ErrorCode.INTERNAL_ERROR, e); } } - - /** - * Get the commit hash for a given reference. This is to construct the commit values for use in things - * like permanent URLs. - * - * @param repositoryRoot the directory to run the git process from - * @param refName a git reference (e.g., branch name, "HEAD", etc.) - * @return the current commit hash - * @throws RelationalException if git commit fails - */ - @Nonnull - public static String getCommitHash(@Nonnull Path repositoryRoot, @Nonnull String refName) throws RelationalException { - try { - final var command = List.of("git", "rev-parse", refName); - final var processBuilder = new ProcessBuilder(command) - .directory(repositoryRoot.toFile()) - .redirectErrorStream(false); - - final var process = processBuilder.start(); - final var exitCode = process.waitFor(); - final String ref = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).strip(); - - if (exitCode != 0) { - throw new RelationalException( - "Git rev-parse command failed with exit code " + exitCode, - ErrorCode.INTERNAL_ERROR); - } - - return ref; - } catch (final IOException | InterruptedException e) { - throw new RelationalException( - "Failed to get current head reference", ErrorCode.INTERNAL_ERROR, e); - } - } } diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java index 2016dae0d0..9463475df4 100644 --- a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java +++ b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/MetricsDiffAnalyzer.java @@ -125,11 +125,9 @@ public static void main(String[] args) { try { final Path repositoryRoot = arguments.repositoryRoot == null ? Paths.get(".") : Paths.get(arguments.repositoryRoot); - final String baseRef = GitMetricsFileFinder.getCommitHash(repositoryRoot, arguments.baseRef); - final String headRef = GitMetricsFileFinder.getCommitHash(repositoryRoot, arguments.headRef); final Path outputPath = arguments.outputPath != null ? Paths.get(arguments.outputPath) : null; - final var analyzer = new MetricsDiffAnalyzer(baseRef, headRef, repositoryRoot, arguments.urlBase); + final var analyzer = new MetricsDiffAnalyzer(arguments.baseRef, arguments.headRef, repositoryRoot, arguments.urlBase); final var analysis = analyzer.analyze(); final var report = analysis.generateReport(); From 84ba2f35b2fdd15d7dd27e57fefd1ec4e05a2aed Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 24 Sep 2025 12:47:45 +0100 Subject: [PATCH 23/24] make metrics_analysis run in prb for test --- .github/workflows/metrics_analysis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 1dd6e946ac..7c5faa7451 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -1,7 +1,7 @@ name: Metrics Diff Analysis on: - pull_request_target: + pull_request: paths: - '**/*.metrics.yaml' @@ -26,7 +26,7 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="${{ github.sha }}" \ + -PmetricsAnalysis.baseRef="4.5.10.0" \ -PmetricsAnalysis.headRef="${{ github.event.pull_request.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \ From b6577167d6a01b0bf0d1a014b99b1872e4e0e120 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 24 Sep 2025 14:03:16 +0100 Subject: [PATCH 24/24] Revert "make metrics_analysis run in prb for test" This reverts commit 84ba2f35b2fdd15d7dd27e57fefd1ec4e05a2aed. --- .github/workflows/metrics_analysis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/metrics_analysis.yml b/.github/workflows/metrics_analysis.yml index 7c5faa7451..1dd6e946ac 100644 --- a/.github/workflows/metrics_analysis.yml +++ b/.github/workflows/metrics_analysis.yml @@ -1,7 +1,7 @@ name: Metrics Diff Analysis on: - pull_request: + pull_request_target: paths: - '**/*.metrics.yaml' @@ -26,7 +26,7 @@ jobs: run: | # Run the analysis. Compare against the base hash of this PR ./gradlew :yaml-tests:analyzeMetrics \ - -PmetricsAnalysis.baseRef="4.5.10.0" \ + -PmetricsAnalysis.baseRef="${{ github.sha }}" \ -PmetricsAnalysis.headRef="${{ github.event.pull_request.head.sha }}" \ -PmetricsAnalysis.urlBase="${{ github.server_url }}/${{ github.repository }}/blob" \ -PmetricsAnalysis.repositoryRoot="${{ github.workspace }}" \