-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Improve verbose and debug logging level messaging in web cmdlets #25510
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
Improve verbose and debug logging level messaging in web cmdlets #25510
Conversation
@iSazonov ready to review! EDIT: I lied, but now codefactor stuff is fixed. |
I wonder how many code you update. 😄 I expect you add only content output. I like you create helper methods. But now output is too large and we must generate the output only it is requested. The same for verbose. But I wouldn't waste time on verbose right now at all. But it's up to you. Also I'd prefer to use standard TosString() for request and response. Why do we create custom implementation? |
@iSazonov I had this discussion with @SeeminglyScience on discord and it is noted there isn't an ideal reliable way to check preference (if you check the first commit I was testing for it prior) and he recommended not establishing it as a pattern but rather this should be something the runtime provides as part of the PSCmdlet abstract class. In testing the code has a non-visible impact even on large payloads if Verbose/Debug aren't specified. I moved away from the Request/Response message EDIT: Also I made very sure not to alter existing Webcmdlet behavior, the additional areas I changed were just additive where they made sense in context, I'm very aware this is some hot-path stuff and I'm keeping the scope just to the output aspects. |
@JustinGrote Yes, we haven't std methods for the checks. I think it's important to have them because later we can add even more output (for example certificates) |
Sounds good. Do we want to merge this PR as-is for the next preview and add the conditional logic later, or wait for #25514? |
Let's wait for what Patrick says. If that PR is not approved, then let's look at switch checks. What I definitely would not like is to create these strings without a condition. |
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.
Mainly style comments.
...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/ContentHelper.Common.cs
Outdated
Show resolved
Hide resolved
...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
Outdated
Show resolved
Hide resolved
@iSazonov thank you for the review, I will incorporate the recently merged PR and your feedback hopefully sometime this week. |
@iSazonov feedback incorporated and it is now conditional using the new internal |
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
64271c7
to
42b6d6a
Compare
@iSazonov changes requested implemented (I went with a nested foreach for the headers to make that easier to read), and made CodeFactor happy. I also squash rebased to a single commit so this should be mergeable now. Let me know any further issues. |
42b6d6a
to
53143d9
Compare
...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
0b31ad7
to
d50453c
Compare
@iSazonov static refactor reverted. CodeFactor however is now complaining about complex methods I didn't touch. I tried a rebase/reset and confirmed I didn't touch those methods as far as I can tell. Any idea what's going on? Is it the nullable maybe? |
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.
Please update the PR description to follow latest changes.
...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/ContentHelper.Common.cs
Outdated
Show resolved
Hide resolved
@iSazonov tests fixed (at least locally on my machine), the Also adjusted encoding test to match on Codepage since we are providing it anyways, should be a much less brittle test. |
@JustinGrote Still hang. I guess VerbosePreference is inquire. |
All retries are done with recursive GetResponse added by PowerShell#19229. The do loop was a leftover from the old way of doing things, it never actually gets triggered, and redirects previously had internal GetResponse that bypassed it, but when I unified the logic to unify the verbose messages, it now follows the flow "properly" and got stuck in the do/while instead of return the response, causing it to reuse the request and trigger an exception.
Fixes test "Invoke-WebRequest -PassThru -OutFile -Verbose File Name reflects the downloaded file name"
This reverts commit f4f446f.
This is still active, there's an issue somewhere in the MaximumRetry code where a request is getting reused, I haven't been able to track it down yet and need to step thru the commits to find where it's happening. |
@iSazonov I found the problem. Please approve for tests to run, it should fully pass now and be mergeable. |
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
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 with one notice: perhaps it makes sense to remove body
from debug output because it can be too large..
It is a possibility with text-based responses, but most extremely large responses will be binary that will be summarized. I will suggest we merge as is and if feedback shows that it is a constant problem, we either add a fixed or user-configurable truncation option. |
@iSazonov is there a reason the linux/windows AzDevOps CI are pending? I don't have any visibility that I know of. |
/azp run PowerShell-CI-linux-packaging,PowerShell-Windows-Packaging-CI |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
else | ||
{ | ||
string friendlyContentLength = ContentHelper.GetFriendlyContentLength( | ||
response.Content?.Headers?.ContentLength); |
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.
Headers is not null here.
if (requestContentLength is not null) | ||
{ | ||
verboseBuilder.Append($" with body size {ContentHelper.GetFriendlyContentLength(requestContentLength)}"); | ||
} | ||
if (OutFile is not null) |
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.
Style: new line is needed after brace.
@JustinGrote Many thanks! |
@iSazonov same to you! |
PR Summary
GetResponse
cognitive loadExample
WG Review Response
Agreed. I have defined the intent of the messages to be as such:
Verbose: One-Liner showing the basics of what is happening, very useful for concisely tracing program flow. Sensitive information has been redacted at this level in general per Security WG comments but URL is preserved for troubleshooting purposes and basic logging flow if necessary.
Debug: Includes all sensitive information including sensitive headers which may include authentication tokens, etc. as the purpose is for debugging, not logging, and specifying DebugPreference or Debug is considered an "opt-in" for this information.
PR Context
Closes #25492
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright header