Skip to content
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

fix: Improve visibility of logging from inside exception handlers #540

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

DaveTryon
Copy link
Contributor

https://github.com/microsoft/dropvalidator/issues/874 calls out some scenarios where error messages were being logged only at Debug verbosity. This PR changes all ILogger.Debug calls in exception handlers to ILogger.Warning to increase the visibility.

@DaveTryon DaveTryon requested a review from a team as a code owner April 9, 2024 18:07
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 17.64706% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 59.73%. Comparing base (85cb4ed) to head (d657938).

Files Patch % Lines
src/Microsoft.Sbom.Api/Output/MetadataBuilder.cs 0.00% 5 Missing ⚠️
...Converters/ExternalReferenceInfoToPathConverter.cs 0.00% 1 Missing ⚠️
...m.Api/Executors/ComponentToPackageInfoConverter.cs 0.00% 1 Missing ⚠️
.../Microsoft.Sbom.Api/Executors/EnumeratorChannel.cs 0.00% 1 Missing ⚠️
src/Microsoft.Sbom.Api/Executors/FileFilterer.cs 0.00% 1 Missing ⚠️
src/Microsoft.Sbom.Api/Executors/FileInfoWriter.cs 0.00% 1 Missing ⚠️
...crosoft.Sbom.Api/Executors/ManifestFileFilterer.cs 0.00% 1 Missing ⚠️
...osoft.Sbom.Api/Executors/ManifestFolderFilterer.cs 0.00% 1 Missing ⚠️
...rosoft.Sbom.Api/Executors/PackageInfoJsonWriter.cs 0.00% 1 Missing ⚠️
...tors/SPDXSBOMReaderForExternalDocumentReference.cs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #540   +/-   ##
=======================================
  Coverage   59.73%   59.73%           
=======================================
  Files         248      248           
  Lines        7572     7572           
  Branches      900      900           
=======================================
  Hits         4523     4523           
  Misses       2637     2637           
  Partials      412      412           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sfoslund sfoslund left a comment

Choose a reason for hiding this comment

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

Changes themselves LGTM. Have you run this version of the tool against any real drops? I would be curious how many of these warnings are surfaced in a normal run, if any

@DaveTryon
Copy link
Contributor Author

Changes themselves LGTM. Have you run this version of the tool against any real drops? I would be curious how many of these warnings are surfaced in a normal run, if any

As we discussed offline, local testing was against the sbom-tool repo where the change was made

@DaveTryon DaveTryon merged commit 09994cd into microsoft:main Apr 9, 2024
6 checks passed
@DaveTryon DaveTryon deleted the revise-logging-verbosity branch April 9, 2024 19:36
@DaveTryon DaveTryon restored the revise-logging-verbosity branch April 9, 2024 20:43
@DaveTryon DaveTryon deleted the revise-logging-verbosity branch April 9, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants