Use results for uploading step summaries #2301

brittanyellich · 2022-12-05T23:33:56Z

This PR introduces a new way to queue and upload step summaries.

yacaovsnc

LGTM!

src/Runner.Worker/FileCommandManager.cs

src/Runner.Common/JobServerQueue.cs

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-06T15:34:59Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+
+namespace GitHub.Services.Results.Client
+{
+    public class ResultsHttpClient : RawHttpClientBase


one thing to check if you plan to use ResultsHttpClient in production for customers, does the RawHttpClientBase support all the network settings for the runner, ex: proxy, SSL cert, etc?

This is for my education only, isn't the RawHttpClientBase the abstraction layer for proxy, SSL certs, retry and all other common http requirements?

it should, but I didn't review the initial PR and am not sure whether we implement all required pieces, it was added to testing RunService only in the dev environment.

Hmmm that is a good question, I'm not familiar with what all the runner requires so I'm not sure if it supports that 🤔 I can look into it and make sure it at least satisfies our needs, though. Good call out!

@yacaovsnc and I chatted and we are both thinking that we should push that effort into another issue, so we can work on incrementally getting this work in and tested. I'll make an issue for this so that we can track the effort.

From what I can tell, RawHttpMessagehandler.cs handles timeouts, retries and has partial support for proxy - it doesn't apply proxy authentication.

It also doesn't support certs which is okay in Results case since we don't need to support self signed certificate yet.

I will try to add the proxy authentication support in the other issue created.

TingluoHuang · 2022-12-08T22:03:06Z

src/Runner.Common/Constants.cs

            public static readonly string UnsupportedSummarySize = "$GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of {0}k, got {1}k. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary";
-            public static readonly string Node12DetectedAfterEndOfLife = "Node.js 12 actions are deprecated. Please update the following actions to use Node.js 16: {0}. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/.";
+            public static readonly string SummaryUploadError = "$GITHUB_STEP_SUMMARY upload aborted, an error occurred when uploading the summary. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary";
+            public static readonly string Node12DetectedAfterEndOfLife = "Node.js 12 actions are deprecated. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/. Please update the following actions to use Node.js 16: {0}";


do you have a bad merge/rebase? the Node12DetectedAfterEndOfLife looks a little bit different.

src/Runner.Common/JobServer.cs

TingluoHuang · 2022-12-08T22:07:09Z

src/Runner.Common/JobServer.cs

+            Trace.Info("Results client is not initialized. Skipping the step summary upload.");
+            return Task.CompletedTask;


do we need to skip or throw?

TingluoHuang · 2022-12-08T22:08:41Z

src/Runner.Common/JobServerQueue.cs

        void Start(Pipelines.AgentJobRequestMessage jobRequest);
        void QueueWebConsoleLine(Guid stepRecordId, string line, long? lineNumber = null);
        void QueueFileUpload(Guid timelineId, Guid timelineRecordId, string type, string name, string path, bool deleteSource);
+        void QueueResultsSummaryUpload(Guid timelineId, Guid timelineRecordId, string type, string name, string path, string stepId, bool deleteSource);


I would suggest renaming to QueueSummaryUpload.

TingluoHuang · 2022-12-08T22:09:52Z

src/Runner.Common/JobServerQueue.cs

        private static readonly TimeSpan _delayForWebConsoleLineDequeue = TimeSpan.FromMilliseconds(500);
        private static readonly TimeSpan _delayForTimelineUpdateDequeue = TimeSpan.FromMilliseconds(500);
        private static readonly TimeSpan _delayForFileUploadDequeue = TimeSpan.FromMilliseconds(1000);
+        private static readonly TimeSpan _delayForResultsSummaryUploadDequeue = TimeSpan.FromMilliseconds(1000);


I would rename everything in this file to not include Result, IJobServerQueue doesn't need to know it's uploading stuff to ResultService.

TingluoHuang · 2022-12-08T22:10:25Z

src/Runner.Common/JobServer.cs

        Task<TaskLog> AppendLogContentAsync(Guid scopeIdentifier, string hubName, Guid planId, int logId, Stream uploadStream, CancellationToken cancellationToken);
        Task AppendTimelineRecordFeedAsync(Guid scopeIdentifier, string hubName, Guid planId, Guid timelineId, Guid timelineRecordId, Guid stepId, IList<string> lines, long? startLine, CancellationToken cancellationToken);
        Task<TaskAttachment> CreateAttachmentAsync(Guid scopeIdentifier, string hubName, Guid planId, Guid timelineId, Guid timelineRecordId, String type, String name, Stream uploadStream, CancellationToken cancellationToken);
+        Task CreateResultsStepSymmaryAsync(string planId, string jobId, string stepId, string file, CancellationToken cancellationToken);


Suggested change

Task CreateResultsStepSymmaryAsync(string planId, string jobId, string stepId, string file, CancellationToken cancellationToken);

Task CreateStepSummaryAsync(string planId, string jobId, string stepId, string file, CancellationToken cancellationToken);

we are creating a summary, and it happens to upload to the ResultService.

TingluoHuang · 2022-12-08T22:14:32Z

src/Runner.Common/JobServerQueue.cs

            _jobServer.InitializeWebsocketClient(serviceEndPoint);

+            // This code is usually wrapped by an instance of IExecutionContext which isn't available here.
+            jobRequest.Variables.TryGetValue("system.github.results_endpoint", out VariableValue resultsEndpointVariable);


should we use the SystemConnection to pass the URL? we have the same pattern for passing CacheServerUrl and GenerateIdTokenUrl from the service to the runner.

What's the difference and the advantage of using SystemConnection?

Going to keep is as a variable for now. The SystemConnction has authorization parameters and for Results we are just sticking with the system access token.

the auth in the SystemConnection is exactly the system access token.
the advantage is aline with the existing pattern, the SystemConnection was holding some connection info back to the service, I think the result service URL is just one of them.

src/Runner.Common/JobServerQueue.cs

TingluoHuang · 2022-12-08T22:16:14Z

src/Runner.Common/JobServerQueue.cs

+            try
+            {
+                Trace.Info($"Starting to upload summary file to results service {file.Type}, {file.Name}, {file.Path}");
+


Suggested change

TingluoHuang · 2022-12-08T22:16:44Z

src/Runner.Common/JobServerQueue.cs

+                Trace.Info($"Starting to upload summary file to results service {file.Type}, {file.Name}, {file.Path}");
+
+
+                if (String.Equals(file.Type, ChecksAttachmentType.ResultsStepSummary, StringComparison.OrdinalIgnoreCase))


will there every be other types of files?

Not planned yet.

TingluoHuang · 2022-12-08T22:18:48Z

src/Runner.Common/JobServerQueue.cs

+                TimelineId = timelineId,
+                TimelineRecordId = timelineRecordId,


do we need these 2 fields? TimelineId and TimelineRecordId

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-08T22:41:38Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+
+        public static async Task<HttpResponseMessage> UploadFileAsync(string url, string blobStorageType, FileStream file, CancellationToken cancellationToken) {
+            // Upload the file to the url
+            var httpClient = new HttpClient();


why do we create a new HttpClient if we are using the RawHttpClientBase? should we use the RawHttpClientBase.SendAsync?

I believe the intention is this upload goes to Blob, not Results - that said, I think the Raw http client should still work.

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-08T22:42:07Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+        public static async Task<HttpResponseMessage> UploadFileAsync(string url, string blobStorageType, FileStream file, CancellationToken cancellationToken) {
+            // Upload the file to the url
+            var httpClient = new HttpClient();
+            var request = new HttpRequestMessage(HttpMethod.Put, url)


also same about dispose.

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-08T22:42:53Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+
+            using (var response = await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken)) {
+                if (!response.IsSuccessStatusCode) {
+                    throw new Exception($"Failed to upload file, status code: {response.StatusCode}, reason: {response.ReasonPhrase}, blobStorageType: {blobStorageType}");


same about whether this will be on user's face.

TingluoHuang · 2022-12-08T22:43:03Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+            }
+        }
+
+        public static async Task<HttpResponseMessage> UploadFileAsync(string url, string blobStorageType, FileStream file, CancellationToken cancellationToken) {


public or private?

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-08T22:43:40Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+        }
+
+        // Handle file upload for step summary
+        public async Task UploadStepSummaryAsync(string jobId, string planId, string stepId, string file, CancellationToken cancellationToken)


order the parameters.

TingluoHuang · 2022-12-08T22:44:05Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+                var fileSize = new FileInfo(file).Length;
+                if (fileSize > uploadUrlResponse.soft_size_limit)
+                {
+                    throw new Exception($"File size is larger than the upload url allows, file size: {fileSize}, upload url size: {uploadUrlResponse.soft_size_limit}, blobType: {uploadUrlResponse.blob_storage_type}");


does the user care about the storage type?

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

TingluoHuang · 2022-12-08T22:45:32Z

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs

+                await StepSummaryUploadCompleteAsync(jobId, planId, stepId, fileSize, cancellationToken);
+            }
+            catch (Exception ex) {
+                throw new Exception("Failed to upload step summary to results", ex);


will this print out a nest error message with tons of stack about the inner exception?

Co-authored-by: Tingluo Huang <tingluohuang@github.com>

fhammerl

LGTM

This reverts commit f41f5d2.

TingluoHuang · 2022-12-19T05:49:21Z

src/Runner.Common/JobServerQueue.cs

+        public Guid TimelineId { get; set; }
+        public Guid TimelineRecordId { get; set; }


I still don't get why we need these fields.

await _jobServer.CreateStepSymmaryAsync(file.PlanId, file.JobId, file.StepId, file.Path, cancellationTokenSource.Token);

Do we only pass PlanId/JobId/StepId/Path to the _jobServer?

* Use results service for uploading step summaries * Use results summary over generic results naming convention * Apply suggestions from code review Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Addressing feedback * Fix merge issue * Remove empty line * Update Results json objects to use snake case * Adding the reference Co-authored-by: Yang Cao <yacaovsnc@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com>

Use results service for uploading step summaries

e5fab8c

brittanyellich requested a review from a team as a code owner December 5, 2022 23:33

yacaovsnc previously approved these changes Dec 6, 2022

View reviewed changes

src/Runner.Worker/FileCommandManager.cs Outdated Show resolved Hide resolved

src/Runner.Common/JobServerQueue.cs Outdated Show resolved Hide resolved

robherley reviewed Dec 6, 2022

View reviewed changes

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs Outdated Show resolved Hide resolved

robherley reviewed Dec 6, 2022

View reviewed changes

src/Sdk/WebApi/WebApi/Results/ResultsHttpClient.cs Outdated Show resolved Hide resolved

TingluoHuang reviewed Dec 6, 2022

View reviewed changes

Use results summary over generic results naming convention

5a9659c

brittanyellich dismissed yacaovsnc’s stale review via 5a9659c December 6, 2022 20:48

brittanyellich requested review from TingluoHuang and yacaovsnc and removed request for TingluoHuang and yacaovsnc December 6, 2022 22:22

yacaovsnc previously approved these changes Dec 6, 2022

View reviewed changes

TingluoHuang reviewed Dec 8, 2022

View reviewed changes

src/Runner.Common/JobServer.cs Outdated Show resolved Hide resolved

TingluoHuang reviewed Dec 8, 2022

View reviewed changes

src/Runner.Common/JobServer.cs Outdated Show resolved Hide resolved

TingluoHuang reviewed Dec 8, 2022

View reviewed changes

src/Runner.Common/JobServerQueue.cs Outdated Show resolved Hide resolved