Post the response from the endpoint when log file posting succeed or failed #2771
Conversation
shaopeng-gh
requested review from
michaelcfanning,
suvamM,
scottoneil-ms,
rwoll,
yongyan-gh,
LingZhou-gh,
EasyRhinoMSFT and
cfaucon
as code owners
| @@ -101,8 +101,20 @@ public static SarifLog Load(Stream source, bool deferred = false) | |||
| return false; | |||
| } | |||
|
|
|||
| await Post(postUri, new MemoryStream(fileBytes), httpClient); | |||
| Console.WriteLine($"Posted log file successfully to: {postUri}"); | |||
| HttpResponseMessage response = await Post(postUri, new MemoryStream(fileBytes), httpClient); | |||
There was a problem hiding this comment.
copied the code section,
but made some changes to the message, please review.
I used generic form: "Endpoint provided message"
src/Sarif/Core/SarifLog.cs
Outdated
| await Post(postUri, new MemoryStream(fileBytes), httpClient); | ||
| Console.WriteLine($"Posted log file successfully to: {postUri}"); | ||
| HttpResponseMessage response = await Post(postUri, new MemoryStream(fileBytes), httpClient); | ||
| string responseText = response.Content.ReadAsStringAsync().Result; |
There was a problem hiding this comment.
This method is actually async, so you can await this Task instead of doing .Result. (And that is better.) #Resolved
| Console.WriteLine($"Error Posting log file to: {postUri}. Endpoint provided message: {responseText}"); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems better to check response.IsSuccessStatusCode. #Closed
There was a problem hiding this comment.
That is, as opposed to doing a try/catch here.
There was a problem hiding this comment.
And I also think status code is useful to user, I think we should add it to the output as well
| return false; | ||
| } | ||
|
|
||
| Console.WriteLine($"Posted log file successfully to: {postUri}. Endpoint provided message: {responseText}"); | ||
| return true; |
There was a problem hiding this comment.
I'm not sure what the format of the responseText is going to be and what it contains. Is it possible to run this locally and provide a sample in the PR description? #Resolved
There was a problem hiding this comment.
This is generic,
the server that takes the SARIF file is to be set by the user,
and that server could be with any design by the user.
It can returns anything, e.g. correlation id.
| return false; | ||
| } | ||
|
|
||
| Console.WriteLine($"Posted log file successfully to: {postUri}. Endpoint provided status code {response.StatusCode} and message: {responseText}"); |
There was a problem hiding this comment.
Thanks. I'm realizing we should revisit this code soon. We probably shouldn't be writing directly to the Console, and instead refactor the APIs at this layer and a few more up above to pass a Context down, so we can access a proper logger. I'll leave it for Michael to decide if we need that in this change. I can see cases in both directions.
The response from the endpoint is useful for troubleshooting.