Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logging system by replacing the global ENABLE_LOGGING flag with a singleton LoggingManager, centralizes logger setup with file rotation and timestamped filenames, and sanitizes connection strings. It also updates logging calls across modules to use the new manager, enhances the C++ binding logging function with formatting and error handling, and adjusts driver path logic for Windows compatibility.
- Introduce
LoggingManagersingleton to manage logging configuration - Replace
ENABLE_LOGGINGchecks withloggerexistence and use newsanitize_connection_string - Update C++
LOGfunction and Windows driver path logic
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_006_logging.py | Updated tests to use LoggingManager and removed global flag |
| mssql_python/logging_config.py | Added LoggingManager class, removed ENABLE_LOGGING, wrapped exports |
| mssql_python/helpers.py | Removed global flag import, added sanitize_connection_string |
| mssql_python/exceptions.py | Replaced global flag checks with logger existence |
| mssql_python/cursor.py | Replaced global flag checks with logger existence |
| mssql_python/connection.py | Initialized logger at import, use sanitize_connection_string |
| mssql_python/pybind/ddbc_bindings.cpp | Enhanced LOG templated function and updated Windows driver path logic |
Comments suppressed due to low confidence (3)
mssql_python/helpers.py:189
- The new
sanitize_connection_stringfunction lacks unit tests. Add tests to verify that sensitive fields likePwdare correctly masked.
def sanitize_connection_string(conn_str: str) -> str:
mssql_python/helpers.py:76
- The
loggervariable is not defined in this module, resulting in a NameError. You should calllogger = get_logger()at the top or fetch a logger inside the function.
if logger:
mssql_python/cursor.py:434
- The
loggervariable is not defined in this module. Definelogger = get_logger()at the top or retrieve the logger in scope before using it.
if logger:
…rosoft/mssql-python into bewithgaurav/enhance_logging
gargsaumya
previously approved these changes
Jul 17, 2025
jahnvi480
previously approved these changes
Jul 17, 2025
gargsaumya
approved these changes
Jul 17, 2025
jahnvi480
approved these changes
Jul 17, 2025
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.
ADO Work Item Reference
Summary
This pull request refactors the logging system in the
mssql_pythonpackage, replacing the previous global logging mechanism with a centralizedLoggingManagerclass. It also introduces a universal logging helper function (log) and a utility to sanitize sensitive information in connection strings. Additionally, it simplifies the codebase by removing redundant logging checks and improving resource management for cursors and connections.Logging System Refactor:
LoggingManageras a singleton class to manage logging configuration, replacing the globalENABLE_LOGGINGvariable. The class centralizes logging setup and provides backward compatibility for checking if logging is enabled. (mssql_python/logging_config.py, mssql_python/logging_config.pyR11-R52)logfunction inhelpers.pyto streamline logging calls across the codebase. This function dynamically retrieves a logger instance and supports multiple log levels. (mssql_python/helpers.py, mssql_python/helpers.pyR187-R214)Code Simplification:
ENABLE_LOGGINGchecks with calls to the newlogfunction, simplifying logging logic inconnection.py,cursor.py, andexceptions.py. (mssql_python/connection.py, [1];mssql_python/cursor.py, [2];mssql_python/exceptions.py, [3]loggerinitialization from several modules, as thelogfunction now handles logger retrieval. (mssql_python/auth.py, [1];mssql_python/cursor.py, [2]New Utilities:
sanitize_connection_stringinhelpers.pyto mask sensitive information (e.g., passwords) in connection strings before logging. (mssql_python/helpers.py, mssql_python/helpers.pyR187-R214)Resource Management Improvements:
mssql_python/connection.py, mssql_python/connection.pyR186)mssql_python/cursor.py, mssql_python/cursor.pyL419-R432)Minor Enhancements:
__del__methods inconnection.pyandcursor.pyto handle exceptions gracefully during cleanup, avoiding issues during garbage collection. (mssql_python/connection.py, [1];mssql_python/cursor.py, [2]