diff --git a/.azure-pipelines/ultimate-pipeline.yml b/.azure-pipelines/ultimate-pipeline.yml index a3c572bf0259..bcd3208ab4d1 100644 --- a/.azure-pipelines/ultimate-pipeline.yml +++ b/.azure-pipelines/ultimate-pipeline.yml @@ -4776,12 +4776,21 @@ stages: env: PR_NUMBER: $(System.PullRequest.PullRequestNumber) AZURE_DEVOPS_TOKEN: $(AZURE_DEVOPS_TOKEN) - GITHUB_TOKEN: $(GITHUB_APP_TOKEN) - publish: $(System.DefaultWorkingDirectory)/artifacts/build_data/execution_benchmarks/execution_time_report.md displayName: Upload report artifact: execution_time_report + # Post the PR comment after the artifact is published so we can link directly to the file. + - script: tracer\build.cmd PostExecutionTimeBenchmarkResultsComment + displayName: Post PR comment with report link + continueOnError: true + env: + PR_NUMBER: $(System.PullRequest.PullRequestNumber) + AZURE_DEVOPS_TOKEN: $(AZURE_DEVOPS_TOKEN) + GITHUB_TOKEN: $(GITHUB_APP_TOKEN) + AZURE_DEVOPS_BUILD_ID: $(Build.BuildId) + - stage: profiler_execution_benchmarks dependsOn: [merge_commit_id, build_windows_profiler, build_linux_profiler] variables: diff --git a/tracer/build/_build/Build.GitHub.cs b/tracer/build/_build/Build.GitHub.cs index a32730429dce..6c8c55928118 100644 --- a/tracer/build/_build/Build.GitHub.cs +++ b/tracer/build/_build/Build.GitHub.cs @@ -911,7 +911,6 @@ await client.Issue.Milestone.Update( .DependsOn(CreateRequiredDirectories) .Requires(() => AzureDevopsToken) .Requires(() => GitHubRepositoryName) - .Requires(() => GitHubToken) .Executes(async () => { var isPr = int.TryParse(Environment.GetEnvironmentVariable("PR_NUMBER"), out var prNumber); @@ -944,14 +943,13 @@ await client.Issue.Milestone.Update( Logger.Information("Markdown build complete, writing report"); - // save the report so we can upload it as an atefact for prosperity + // save the report so we can upload it as an artifact for prosperity await File.WriteAllTextAsync(executionDir / "execution_time_report.md", markdown); - if(isPr) - { - Logger.Information("Updating PR comment on GitHub"); - await ReplaceCommentInPullRequest(prNumber, "## Execution-Time Benchmarks Report", markdown); - } + // save a concise summary for the PR comment (posted by the next step, after the artifact is published) + var summaryMarkdown = CompareExecutionTime.GetCommentSummary(sources); + Logger.Information("Summary build complete, writing comment summary"); + await File.WriteAllTextAsync(executionDir / "execution_time_summary.md", summaryMarkdown); async Task GetExecutionBenchmarkArtifacts(BuildHttpClient httpClient, string branch, AbsolutePath directory) { @@ -980,6 +978,97 @@ await client.Issue.Milestone.Update( } }); + /// + /// Posts the execution-time benchmark comparison as a PR comment, with a direct link to the + /// full report artifact. Must run after the execution_time_report artifact has + /// been published so that the single-file download URL can be resolved. + /// + Target PostExecutionTimeBenchmarkResultsComment => _ => _ + .Unlisted() + .Requires(() => AzureDevopsToken) + .Requires(() => GitHubToken) + .Requires(() => AzureDevopsBuildId) + .Executes(async () => + { + var isPr = int.TryParse(Environment.GetEnvironmentVariable("PR_NUMBER"), out var prNumber); + if (!isPr) + { + Logger.Information("Not a PR build, skipping comment posting"); + return; + } + + var executionDir = BuildDataDirectory / "execution_benchmarks"; + var summaryPath = executionDir / "execution_time_summary.md"; + + if (!File.Exists(summaryPath)) + { + throw new Exception($"No execution time summary found at {summaryPath}, skipping comment"); + } + + var summaryMarkdown = await File.ReadAllTextAsync(summaryPath); + + // Resolve the single-file download URL for the execution_time_report artifact. + // This requires the artifact to already be published (guaranteed by pipeline step ordering). + string reportUrl = null; + var connection = new VssConnection( + new Uri(AzureDevopsOrganisation), + new VssBasicCredential(string.Empty, AzureDevopsToken)); + + using var buildHttpClient = connection.GetClient(); + + // Retry a few times: artifact registration can lag the publish step by a moment. + for (var attempt = 0; attempt < 5 && reportUrl is null; attempt++) + { + if (attempt > 0) + { + await Task.Delay(TimeSpan.FromSeconds(5 * attempt)); + Logger.Information("Retrying artifact lookup (attempt {Attempt}/5)", attempt + 1); + } + + try + { + var artifact = await buildHttpClient.GetArtifactAsync( + project: AzureDevopsProjectId, + buildId: AzureDevopsBuildId.Value, + artifactName: "execution_time_report"); + + // Convert the zip downloadUrl to a single-file download URL. + reportUrl = artifact.Resource.DownloadUrl + .Replace("?format=zip", "?format=file&subPath=/execution_time_report.md"); + + Logger.Information("Resolved report URL: {Url}", reportUrl); + } + catch (ArtifactNotFoundException) + { + Logger.Information("Artifact not yet available (attempt {Attempt}/5)", attempt + 1); + } + catch (VssServiceException ex) + { + Logger.Information(ex, "Error looking up artifact (attempt {Attempt}/5)", attempt + 1); + } + } + + string reportLink; + if (reportUrl is null) + { + // Fall back to the build's artifacts page: not a direct link to the report, but it's + // deterministic, so we can always give people _some_ way to get to the full report. + var artifactsPageUrl = $"{AzureDevopsOrganisation}/{GitHubRepositoryName}/_build/results?buildId={AzureDevopsBuildId.Value}&view=artifacts&pathAsName=false&type=publishedArtifacts"; + Logger.Warning("Could not resolve single-file report URL, linking to the build artifacts page instead"); + reportLink = $"๐Ÿ“„ **[Download the full report from the build artifacts โ†’]({artifactsPageUrl})**"; + } + else + { + var viewerUrl = $"https://andrewlock.github.io/merview/?zen=1&url={Uri.EscapeDataString(reportUrl)}"; + reportLink = $"๐Ÿ“„ **[View the full report (charts + all metrics) โ†’]({viewerUrl})**"; + } + + var fullMarkdown = summaryMarkdown + "\n\n" + reportLink; + + Logger.Information("Updating PR comment on GitHub"); + await ReplaceCommentInPullRequest(prNumber, "## Execution-Time Benchmarks Report", fullMarkdown); + }); + Target VerifyReleaseReadiness => _ => _ .Unlisted() .Requires(() => GitHubToken) diff --git a/tracer/build/_build/ExecutionTimeComparison/CompareExecutionTime.cs b/tracer/build/_build/ExecutionTimeComparison/CompareExecutionTime.cs index c8790d9633df..adecc010d8da 100644 --- a/tracer/build/_build/ExecutionTimeComparison/CompareExecutionTime.cs +++ b/tracer/build/_build/ExecutionTimeComparison/CompareExecutionTime.cs @@ -21,38 +21,54 @@ public static string GetMarkdown(List sources) Logger.Information("Reading execution benchmarkResults results"); var results = sources.SelectMany(ReadJsonResults).ToList(); - // Group execution time benchmarks by Sample Name, Framework + // Group execution time benchmarks by Sample Name; show all frameworks inline within each group Logger.Information($"Found {results.Count} results: building markdown"); var charts = results - .GroupBy(x => (x.TestSample, x.Framework)) - .Select(group => + .GroupBy(x => x.TestSample) + .OrderBy(g => g.Key) + .Select(sampleGroup => { - var scenarios = group - .Select(x => x) - .OrderBy(x => x.Scenario == "Baseline" ? 0 : 1 ) - .ThenBy(x => x.Scenario) - .GroupBy(x => x.Scenario) - .Select((scenarioResults, i) => GetMermaidSection(scenarioResults.Key, scenarioResults)); - var chartTitle = $"{group.Key.TestSample} ({GetName(group.Key.Framework)})"; + var sampleName = sampleGroup.Key.ToString(); + var frameworkCharts = sampleGroup + .GroupBy(x => x.Framework) + .OrderBy(g => g.Key) + .Select(frameworkGroup => + { + var frameworkName = GetName(frameworkGroup.Key); + var scenarios = frameworkGroup + .OrderBy(x => x.Scenario == "Baseline" ? 0 : 1) + .ThenBy(x => x.Scenario) + .GroupBy(x => x.Scenario) + .Select((scenarioResults, i) => GetMermaidSection(scenarioResults.Key, scenarioResults)); + return $""" + ```mermaid + gantt + title Execution time (ms) {sampleName} ({frameworkName}) + dateFormat x + axisFormat %Q + todayMarker off + {string.Join(Environment.NewLine, scenarios)} + ``` + """; + }); return $""" -
- {chartTitle} - - ```mermaid - gantt - title Execution time (ms) {chartTitle} - dateFormat x - axisFormat %Q - todayMarker off - {string.Join(Environment.NewLine, scenarios)} - ``` +
+

{sampleName}

+ + {string.Join(Environment.NewLine + Environment.NewLine, frameworkCharts)}
"""; }); + var sampleNames = results + .Select(x => x.TestSample.ToString()) + .Distinct() + .OrderBy(x => x) + .ToList(); + var comparisonTable = GetComparisonTable(results); - return GetCommentMarkdown(sources, charts, comparisonTable); + return GetCommentMarkdown(sources, charts, comparisonTable, sampleNames); } static EquivalenceTestConclusion CalculateSignificance(double[] masterValues, double[] currentValues) @@ -193,7 +209,72 @@ static string GetMermaidSection(string scenario, IEnumerable + /// Returns a concise summary of the comparison results for use in a PR comment. + /// Contains only the regressions table (no Mermaid charts, no full metrics details), + /// so that the comment remain small. The caller is expected to append a direct + /// link to the full report artifact. + /// + public static string GetCommentSummary(List sources) + { + Logger.Information("Reading execution benchmark results for comment summary"); + var results = sources.SelectMany(ReadJsonResults).ToList(); + + Logger.Information($"Found {results.Count} results: building comment summary"); + var (regressionsMarkdown, _, hasRegressions) = BuildComparisonSections(results); + + var regressionsSummary = hasRegressions + ? "### โš ๏ธ Potential regressions detected\n\n" + regressionsMarkdown + : "โœ… No regressions detected"; + + return $$""" + ## Execution-Time Benchmarks Report โฑ๏ธ + + Execution-time results for samples comparing {{string.Join(" and ", sources.Select(x => x.Markdown))}}. + + {{regressionsSummary}} + """; + } + static string GetComparisonTable(List results) + { + var (regressionsMarkdown, detailsMarkdown, hasRegressions) = BuildComparisonSections(results); + + var finalOutput = new StringBuilder(); + + finalOutput.AppendLine("

Comparison Results

"); + finalOutput.AppendLine(); + + if (hasRegressions) + { + finalOutput.AppendLine("โš ๏ธ Potential regressions detected"); + finalOutput.AppendLine(); + finalOutput.Append(regressionsMarkdown); + } + else + { + finalOutput.AppendLine("โœ… No regressions detected"); + finalOutput.AppendLine(); + } + + finalOutput.AppendLine("
"); + finalOutput.AppendLine("

Full Metrics Comparison

"); + finalOutput.AppendLine(); + finalOutput.Append(detailsMarkdown); + finalOutput.AppendLine("
"); + + return finalOutput.ToString(); + } + + /// + /// Builds both the regressions-only and full-details comparison sections from the given results. + /// + /// + /// regressionsMarkdown: HTML table rows for regressions only (no surrounding header). + /// detailsMarkdown: HTML table rows for all metrics (no surrounding <details>). + /// hasRegressions: whether any statistically-significant regressions were found. + /// + static (string regressionsMarkdown, string detailsMarkdown, bool hasRegressions) BuildComparisonSections(List results) { // Key metrics to compare var keyMetrics = new[] @@ -337,7 +418,7 @@ static string GetComparisonTable(List results) // Build table for this sample in details if (detailsTableRows.Length > 0) { - detailsOutput.AppendLine($"

{sampleName}

"); + detailsOutput.AppendLine($"

{sampleName}

"); detailsOutput.AppendLine(""); detailsOutput.AppendLine(" "); detailsOutput.AppendLine(" "); @@ -378,28 +459,7 @@ static string GetComparisonTable(List results) } } - var finalOutput = new StringBuilder(); - - if (hasRegressions) - { - finalOutput.AppendLine("### โš ๏ธ Potential regressions detected"); - finalOutput.AppendLine(); - finalOutput.Append(regressionsOutput); - } - else - { - finalOutput.AppendLine("โœ… No regressions detected - check the details below"); - finalOutput.AppendLine(); - - } - - finalOutput.AppendLine("
"); - finalOutput.AppendLine(" Full Metrics Comparison"); - finalOutput.AppendLine(); - finalOutput.Append(detailsOutput); - finalOutput.AppendLine("
"); - - return finalOutput.ToString(); + return (regressionsOutput.ToString(), detailsOutput.ToString(), hasRegressions); } static (string html, bool isRegression) FormatMetricRowFromStats( @@ -470,17 +530,29 @@ static string FormatMetricValue(string metricName, double mean, double ci95Lower : ("โœ…", false); // Improvement (faster/lower is better) } - static string GetCommentMarkdown(List sources, IEnumerable charts, string comparisonTable) + static string GetCommentMarkdown(List sources, IEnumerable charts, string comparisonTable, IReadOnlyList sampleNames) { + var subLinks = (string section) => + string.Join(" ยท ", sampleNames.Select(n => $"[{n}](#{n.ToLower()}-{section})")); + + var toc = $""" + - [Comparison Results](#comparison-results) + - [Full Metrics Comparison](#full-metrics-comparison) โ€” {subLinks("metrics")} + - [Comparison Explanation](#comparison-explanation) + - [Duration Charts](#duration-charts) โ€” {subLinks("charts")} + """; + return $$""" - ## Execution-Time Benchmarks Report :stopwatch: +

Execution-Time Benchmarks Report โฑ๏ธ

Execution-time results for samples comparing {{string.Join(" and ", sources.Select(x => x.Markdown))}}. + {{toc}} + {{comparisonTable}}
- Comparison explanation + Comparison Explanation

Execution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. @@ -498,8 +570,10 @@ static string GetCommentMarkdown(List sources, IEnume

-
- Duration charts + --- + +
+

Duration Charts

{{string.Join('\n', charts)}}
""";