Added StackTrace multiple locations in source code to assist when troubleshooting #1468
Conversation
tkol2022
changed the title
Tested scenario - AAD ExportCapPolicies returns empty tableExpected result: make sure warning message displays, execution does not stop. No errors are thrown in this scenario.
|
Tested scenario - AAD ExportCapPolicies returns invalid JSONExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop.
|
Tested scenario - AAD Error occurs inside Get-PrivilegedUserExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report indicates that the commands failed in the respective policies.
Second scenario - Error occurs inside a Get-PrivilegedUser nested functionExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report indicates that the commands failed in the respective policies.
|
Tested scenario - AAD Error occurs inside Get-PrivilegedRoleExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report indicates that the commands failed in the respective policies.
Second scenario - occurs inside Get-PrivilegedRole nested functionExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report indicates that the commands failed in the respective policies.
|
Tested scenario - Error occurs inside AAD Get-TenantDetailExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop.
|
Tested scenario - Defender Error occurs inside top level code - Invalid or missing CmdletExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective policy error.
|
Tested scenario - Defender Error occurs connecting to ExchangeOnlineExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop.
|
Tested scenario - Defender Error occurs connecting to Connect-IPPSSessionExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report displays respective errors.
|
Tested scenario - EXO Error occurs inside top level code - Invalid or missing CmdletExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective policy error.
|
Tested scenario - EXO Error occurs inside Get-EXOTenantDetailExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective Tenant ID error.
|
Tested scenario - Sharepoint Error occurs inside top level code - Invalid or missing CmdletExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective policy error.
|
Tested scenario - Teams Error occurs inside top level code - Invalid or missing CmdletExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective policy error.
Second scenario - Teams Error occurs inside nested function Get-TeamsTenantDetailExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective error.
|
Tested scenario - PowerPlatform Error occurs inside top level code - Invalid or missing CmdletExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective policy error.
Second scenario - PowerPlatform Error occurs inside Get-PowerPlatformTenantDetailExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop. Report contains respective error.
|
Tested scenario - Connection > Disconnect-SCuBATenantExpected result: make sure warning message displays with the error message, stacktrace.
|
Tested scenario - Connection > Connect-TenantExpected result: make sure warning message displays with the error message, stacktrace.
|
…fixed stacktrace with aad, commandtracker
Tested scenario - Orchestrator > Invoke-ProviderList - inner try catch {} nested in foreach loopExpected result: make sure warning message displays with the error message, stacktrace when executing one of the providers. Execution continues.
Tested scenario - Orchestrator > Invoke-ProviderList - outer try catch {}Expected result: make sure custom message displays with the error message, stacktrace and execution stops because completing the provider is a critical task.
|
Tested scenario - Orchestrator > Invoke-RunRego - All products failed inside foreach loopExpected result: make sure warning message displays with the error message, stacktrace and execution stops since all providers failed.
Tested scenario - Orchestrator > Invoke-RunRego - A single product failed inside foreach loopExpected result: make sure warning message displays with the error message, stacktrace and execution continues since only a single provider failed.
Tested scenario - Orchestrator > Invoke-RunRego - Error occurred in outer catch {} blockExpected result: make sure custom message displays with the error message, stacktrace and execution stops since creating the Rego results json file is critical.
|
Tested scenario - Orchestrator > ConvertTo-ResultsCsv - All products failedExpected result: make sure warning message displays with the error message, stacktrace and execution does not stop since creating the CSV is not a critical task.
|
TODO - Future enhancementsThe following items were observed as I was working this PR that I feel would be some good enhancements to the error handling logic in future issues. Item 1 - Prevent duplicate error message displays from Merge-JsonOutputWhen an error occurs in this function, it gets displayed twice in the console which really clouds up the output. It would be ideal to only display the same error message once. I believe this occurs because there is no catch {} block in the calling function Invoke-Scuba so what ends up happening is that the thrown error gets displayed a second time by Powershell itself.
Item 2 - Write powershell unit tests to exercise all the various exception blocks and ensure that they display the stacktrace and error message.Item 3 - In Orchestrator > Invoke-ProviderList if ALL of the providers fail, the Orchestrator should stop execution instead of trying to proceed.Item 4 - We should probably standardize when we display an error message using Write-Warning versus Write-Error to be consistent |
Tested scenario - Orchestrator > Merge-JsonOutput - path length error occurredExpected result: make sure custom path message displays with the error message, stacktrace and execution stops since merging the JSON is a critical task.
Second scenario - Orchestrator > Merge-JsonOutput - generic error occurredExpected result: make sure generic message displays with the error message, stacktrace and execution stops since merging the JSON is a critical task.
|
Tested scenario - Orchestrator > Invoke-ReportCreation - error occurredExpected result: make sure custom message displays with the error message, stacktrace and execution stops since creating the report is a critical task.
|
Tested scenario - Orchestrator > Import-Resources - error occurredExpected result: make sure custom message displays with the error message, stacktrace and execution stops since importing modules is a critical task.
|
Tested scenario - RunRego.psm1 > Invoke-Rego - error occurredExpected result: I removed the try catch logic from this function since the calling function already has it so make the error message and stacktrace display and execution stops since invoking Rego is a critical task.
|
…est for ConvertTo-ResultsCsv
tkol2022
added
the
enhancement
tkol2022
had a problem deploying
to
Development
GitHub Actions
Failure
There was a problem hiding this comment.
Completed code review and all looked good, with exception of wanting a format change to how stacktraces are shown. Makes for a single warning with the exception message, but adding newline between message and trace for clearer flow, particularly when there are multiple traces.
Will review operational test cases as part of review part 2 separately, but wanted to get these in ASAP.
| Write-Warning "Error establishing a connection with $($Product): $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
I would recommend a slightly different format for showing the stacktrace to make it more consistent with the way they are typically shown in other programs. In those cases, the stacktrace (when included) immediately follows and is part of the exception message in a format like:
Exception Message
StackTrace
This makes it easier to pair the trace with the error message, and prevents additional WARNING: prepends on the stacktrace. So it's easy to see where the trace begins and ends. The new format could look like this:
| Write-Warning "Error establishing a connection with $($Product): $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error establishing a connection with $($Product): $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
Here's an example of what that looks like
This would also apply to all occurrences for consistency.
| Write-Warning "ERROR: Could not disconnect from $Product`n: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
I know the ERROR was there previously, but since this is actually a warning AND since it's already pre-pended with WARNING, I'd recommend removing the 'ERROR:' bit as it seems contradictory and redundant.
Also including previously mentioned format change recommendation.
| Write-Warning "ERROR: Could not disconnect from $Product`n: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Could not disconnect from $Product`n: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Write-Warning "Error with the $($BaselineName) Provider: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error with the $($BaselineName) Provider: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error with the $($BaselineName) Provider: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Ending ScubaGear execution. Error: $($_.Exception.Message) ` | ||
| Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation, because of the line break not 100% this works as shown without some tweaking.
| Ending ScubaGear execution. Error: $($_.Exception.Message) ` | |
| Stacktrace: $($_.ScriptStackTrace)" | |
| Ending ScubaGear execution. Error: $($_.Exception.Message)` | |
| `n$($_.ScriptStackTrace)" |
| Write-Warning "Error with the $($BaselineName) Rego invocation: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error with the $($BaselineName) Rego invocation: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error with the $($BaselineName) Rego invocation: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Write-Warning "Error retrieving Tenant details using Get-EXOTenantDetail: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error retrieving Tenant details using Get-EXOTenantDetail: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error retrieving Tenant details using Get-EXOTenantDetail: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Write-Warning "Error retrieving Tenant details using Get-PowerPlatformTenantDetail: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error retrieving Tenant details using Get-PowerPlatformTenantDetail: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error retrieving Tenant details using Get-PowerPlatformTenantDetail: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Write-Warning "Error retrieving Tenant details in Get-TeamsTenantDetail: $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error retrieving Tenant details in Get-TeamsTenantDetail: $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error retrieving Tenant details in Get-TeamsTenantDetail: $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| Write-Warning "Error running $($Command): $($_.Exception.Message)" | ||
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" |
There was a problem hiding this comment.
Format update recommendation
| Write-Warning "Error running $($Command): $($_.Exception.Message)" | |
| Write-Warning "Stacktrace: $($_.ScriptStackTrace)" | |
| Write-Warning "Error running $($Command): $($_.Exception.Message)`n$($_.ScriptStackTrace)" |
| @@ -85,7 +85,7 @@ InModuleScope Orchestrator { | |||
| } | |||
| { ConvertTo-ResultsCsv @CsvParameters} | Should -Not -Throw | |||
| Should -Invoke -CommandName Format-PlainText -Exactly -Times 0 | |||
| Should -Invoke -CommandName Write-Warning -Exactly -Times 1 | |||
| Should -Invoke -CommandName Write-Warning -Exactly -Times 2 | |||
There was a problem hiding this comment.
With the format update recommendation, this would likely revert back to 1 Times since there would be a single Write-Warning call to print both the exception message and stack trace.
| Should -Invoke -CommandName Write-Warning -Exactly -Times 2 | |
| Should -Invoke -CommandName Write-Warning -Exactly -Times 1 |
I'd recommend we link these TODOs to the follow-on reformulated issue related to the existing #1344. Mentioning here so the reference is in #1344 when it gets reviewed for closure and reformulation. |
|
I think the solution here is a separate catch block specifically for the type of exception thrown when max file length is exceeded. That catch block can include what's in the if statement and shed the outer exception message while still appending the stacktrace, then a general catch block that has the else statement. That's cleaner than trying to if/else since the catch mechanism already supports conditional catches based on error type.
|
There was a problem hiding this comment.
Review Part 2 - Performed operational testing similar to the scenarios @tkol2022 put forth which were thorough. Spot checked redo of a few and got the same results as @tkol2022 which were as expected. Found one additional subordinate function to Invoke-SCuBA in Test case 3 below for revision to support consistency of error reporting format/feedback. See below.
Added the following scenarios which also performed as expected:
Test Scenario 1 - Invoking PowerPlatform with incorrect M365Environment (commercial) on G5 tenant
Resulted in throwing the correct warning and then failing ScubaGear overall as this was the only product selected for the run.
Test Scenario 2 - Compare-ProductList failure in Orchestrator Invoke-Scuba
This one causes Compare-ProductList function to throw an exception inside the Invoke-Scuba main try block. Since function only runs if a provider command fails, second throw added to TryCommand to force execution of Compare-ProductList code path. Results did not match expected format as other functions called directly from Orchestrator catch and emit their own exceptions.
Recommendation is to modify the Compare-ProductList function to catch and emit exception message and stacktrace rather than throwing its error for higher level handling.
There was a problem hiding this comment.
Did multiple tests with following simulated errors: incorrect M365 environment, errors in get priv user roles, errors in connecting exo, opa path error, convert to csv errors.
In each case, the stack trace showed expected error trace.
Apart from Addam's suggestion for updating warning format, no new suggested changes from me. Nice work.
🗣 Description
Added the Powershell error stack trace to numerous locations in the source code to assist when troubleshooting bug reports.
I removed the try catch {} blocks in the following functions since the calling functions handle the stacktrace now: Get-PrivilegedUser, Get-PrivilegedRole, Invoke-Rego.
Closes #1344
💭 Motivation and context
🧪 Developer Testing
I tested an error scenario or two for each of the functions that I modified to add the stacktrace and verified that the error information was displayed as expected. Results are documented in this PR.
🧪 Instructions for Reviewers
✅ Pre-approval checklist
✅ Pre-merge checklist
PR passed smoke test check.
Feature branch has been rebased against changes from parent branch, as needed
Use
Rebase branchbutton below or use this reference to rebase from the command line.Resolved all merge conflicts on branch
Notified merge coordinator that PR is ready for merge via comment mention
Demonstrate changes to the team for questions and comments.
(Note: Only required for issues of size
Mediumor larger)✅ Post-merge checklist