Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

(#3258) Expand logging for nuget resources errors #3262

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Jul 11, 2023

Description Of Changes

  • When resolving resources for a repository, take the initial warning we have and then inspect its inner exceptions.
  • Log all exception messages found this way to Debug, except for the last one which is logged as a warning again.

The reason the outermost and innermost exceptions are logged as warnings and the rest are relegated to debug is so that we have the best chance of giving users the most actionable information -- often the surface or the deepest exception contain the real problem, while the ones in the middle are often not as useful. However, should we encounter cases where the others are needed, users can inspect the log file for the additional messages, or use --debug to see them on a re-run.

Motivation and Context

Previously all that was logged was the surface exception, which in quite a few cases we've seen not yielding any useful information.

Instead, log the entire exception chain, surfacing just the outermost and innermost exceptions, which we generally expect to contain the most relevant information for users.

Testing

Reproducing the exact cases we've seen this thus far seemed non-trivial and needing quite a bit of setup to me, so I found this trivial case that illustrates the same behaviour:

  1. Run choco search chocolatey --source https://www.google.com/ --proxy www.google.com
  2. You should get two warnings; one that says unable to load the service index, and another that says the remote server returned an error (405)
  3. Rerun the command with --debug and note there is an additional (in this case, not useful) exception logged between the two warnings that simply says an error occurred while sending the request

Operating Systems Testing

Win11

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation. - the shortlink links to a doc page that is to-be-written
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes #3258

@vexx32 vexx32 requested review from gep13 and corbob July 11, 2023 20:47
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few formatting things.

@corbob corbob force-pushed the 3258-exception-logging branch from fc8c614 to d73cf0b Compare July 11, 2023 23:59
corbob and others added 2 commits July 11, 2023 17:01
Previously all that was logged was the surface exception, which in quite
a few cases we've seen not yielding any useful information.

Instead, log the entire exception chain, surfacing just the outermost
and innermost exceptions, which we generally expect to contain the most
relevant information for users.
@corbob corbob force-pushed the 3258-exception-logging branch from d73cf0b to cbfa4d9 Compare July 12, 2023 00:01
@corbob corbob merged commit 9c470f1 into chocolatey:develop Jul 12, 2023
@corbob
Copy link
Member

corbob commented Jul 12, 2023

Thanks for getting this sorted out @vexx32 👍

@vexx32 vexx32 deleted the 3258-exception-logging branch July 12, 2023 12:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand logging around the exception that is thrown when Chocolatey CLI is unable to communicate with a source
2 participants