-
Notifications
You must be signed in to change notification settings - Fork 958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uploading step logs to Results as well #2422
Uploading step logs to Results as well #2422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 🚀 Just a bunch of small things. Mostly grammar tweaks to make it easier to read any logs/messages
} | ||
else if (String.Equals(file.Type, CoreAttachmentType.ResultsLog, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
if (file.RecordId != _jobTimelineRecordId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to only upload step logs vs job level logs? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will relax this in next epic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I would potentially just add a comment that for now we're only uploading step logs and job logs will be addressed in the future
LineCount = lineCount, | ||
}; | ||
|
||
var stepLogsUploadCompleteRequest = new Uri(m_resultsServiceUrl, "twirp/results.services.receiver.Receiver/CreateStepLogsMetadata"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on putting "twirp/results.services.receiver.Receiver/CreateStepLogsMetadata"
, "twirp/results.services.receiver.Receiver/GetStepLogsSignedBlobURL"
and "twirp/results.services.receiver.Receiver/CreateStepSummaryMetadata"
into some list of constants at the bottom of the file? Maybe even a separate file.
I imagine in the future we'll have more of these types of URLs so a single place for all of them now might make sense 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me sit on this for a bit. I do want to encapsulate all the logic in a single method, so I don't have to jump around when reading code. We should pull this out to a common place if it's used in multiple places. Let me see if any other places would benefit from referencing those consts.
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -85,6 +112,40 @@ private async Task StepSummaryUploadCompleteAsync(string planId, string jobId, s | |||
} | |||
} | |||
|
|||
private async Task StepLogUploadCompleteAsync(string planId, string jobId, Guid stepId, long lineCount, CancellationToken cancellationToken) | |||
{ | |||
var timestamp = DateTime.UtcNow.ToString("yyyy-MM-dd'T'HH:mm:ss.fffK"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider using constants. There's a few areas in this file with hard coded values. We have a central file for runner constants you could create your own section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I will need to make a runner PR again very soon for uploading job logs. 😅
I will move those hardcoded values to the const file then.
This reverts commit e979331.
This reverts commit f6c3fd8.
actions#2437)" This reverts commit 8c096ba.
* Rename queue to results queue * Add results contracts * Add Results logging handling * Adding calls to create and finalize append blob * Modifications for azurite upload * Only call upload complete on final section and remove size * Make method specific to step log so we can support job log later * Change contract for results * Add totalline count to the result log upload file * Actually pass lineCount to Results Service * Fix typos * Code cleanup * Fixing typos * Apply suggestions from code review Co-authored-by: Konrad Pabjan <konradpabjan@github.com> --------- Co-authored-by: Brittany Ellich <brittanyellich@github.com> Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
* Revert "Revert "Uploading step logs to Results as well (actions#2422)" (actions#2437)" This reverts commit 8c096ba. * Properly guard the upload to results feature * Delete skipped file if deletesource is true
* Revert "Revert "Uploading step logs to Results as well (actions#2422)" (actions#2437)" This reverts commit 8c096ba. * Properly guard the upload to results feature * Delete skipped file if deletesource is true
Also create step logs in Results. Best effort.