Improve logging configuration and failure diagnosability - #60
Merged
Conversation
…tests for failure diagnosability
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Runs were noisy by default yet hard to debug after the fact: the import-time
basicConfigprinted rawINFO/DEBUGlines straight over the Rich step UI, several modules pinned their own loggers toINFO(soDEBUGnever surfaced anywhere), and an unexpected failure collapsed to a singleUnexpected error: ...line with no traceback and no log file. On top of that,sync_assessmentraised where every sibling stage returned{"success": ...}, so an online-sync failure exitedUNEXPECTEDinstead ofRISK_ASSESSMENT, and the AWS inventory swallowed per-service errors silently — a throttled or access-denied service looked identical to an empty one. This PR makes the console quiet by default but fully diagnosable: verbosity is opt-in via-v/-vv, every run writes a completerun.log, log levels are re-leveled to mean something, and unexpected crashes now leave an actionable traceback behind.What changed
main.py— replaced the import-timebasicConfigwithconfigure_logging(), which routes all logs through a Rich handler and gates the console level on a-vcount:WARNINGby default,INFOat-v,DEBUG+ third-party at-vv.-v/--verboseis exposed on both theawsandazuresubcommands via a shared parent parser. Addedadd_run_log_handler(), which always writes a DEBUGrun.logto the report directory (guarded so a logging-setup failure can never abort a run). Third-party loggers (botocore,boto3,azure) stay muted until-vv;PIL's per-PNG-chunk tracing is pinned toWARNINGalways. The unexpected-error handler now writes the full traceback toerror-<ts>.login the report directory (falling back to the cwd if the crash predates the directory), prints its path, and also funnels it torun.log.core/engine.py—sync_assessmentnow returns the{"success": ...}contract its sibling stages use instead of raising in its three failure paths, so online-sync failures exitRISK_ASSESSMENTrather thanUNEXPECTED. Expected credential/permission-validation failures dropped fromERRORtoWARNING(genuine unexpected errors stayERROR).core/utils_aws.py— the per-serviceexceptnow logs atDEBUG(which service/operation/region + the error) and continues, so a throttled/denied/region-unavailable service is distinguishable from an empty one inrun.logwithout flooding the console.core/utils_db.py,core/utils_azure.py— low-level helpers that re-raise now log atDEBUGinstead ofERROR, so the stage boundary emits the singleERROR(with traceback) rather than double-logging.utils/sync.py,utils/connection.py— expected "not configured / offline skip" conditions dropped toDEBUG; addedexc_info=Trueto the real request/JSON failures that lacked it.utils/azure.py,utils/utils.py— input-validation retry-loop messages dropped fromWARNINGtoDEBUG(the prompt already re-asks visibly).core/utils_db.py,core/utils_sync.py,core/utils_report.py,core/utils_report_json.py,core/utils_report_egress.py,core/utils_report_pdf.py,core/utils_report_html.py— removed seven per-modulesetLevel(logging.INFO)pins that were swallowingDEBUGat the source and defeatingrun.log; filtering is now the handlers' job.tests/test_engine.py,tests/test_utils_and_main.py,tests/test_utils_aws.py— added 6 tests:sync_assessment's return contract (offline / server failure / local-DB failure all return, never raise), the unexpected-error traceback file, the online-sync →RISK_ASSESSMENTexit path, and the AWS per-service skip being logged atDEBUGonly (asserts nothing is emitted at≥ WARNING). Full suite: 215 passing, black-clean.Notes for reviewers
WARNING(was effectivelyINFO), so rawINFO/DEBUGlines no longer print on a normal run — the Rich step UI plus warnings/errors remain.RISK_ASSESSMENTinstead ofUNEXPECTED.run.logcapturesDEBUGpayload dumps (API result + outgoing sync payload); confirmed these carry no secrets.