feat: Adding extensive loggers #224
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes introduce logging functionality to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
Client->>API: GET request
API-->>Client: Response
Client->>Client: Log response
Client->>API: POST request
Client->>Client: Log payload
API-->>Client: Response
Client->>Client: Log response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
sage_desktop_sdk/core/client.py (4)
125-126: Improve logging practices for security and consistencyWhile adding logging is beneficial, consider the following improvements:
On line 125, avoid logging the full response text as it may contain sensitive information. Instead, log only necessary details or mask sensitive data.
On line 138, use
request_urlinstead ofurlto log the full URL for consistency with the debug log.Consider applying these changes:
-logger.debug('Response for get request for url: %s, %s', request_url, response.text) +logger.debug('Response for get request for url: %s, status: %s', request_url, response.status_code) -logger.info('Response for get request for url: %s, %s', url, err.response.text) +logger.info('Response for get request for url: %s, status: %s', request_url, err.response.status_code)Also applies to: 138-138
170-170: Improve logging practices for security and consistencySimilar to the previous comment, consider the following improvements:
On line 170, avoid logging the full response text as it may contain sensitive information.
On line 174, use
request_urlinstead ofurlto log the full URL for consistency.Consider applying these changes:
-logger.debug('Response for get request for url: %s, %s', request_url, response.text) +logger.debug('Response for get request for url: %s, status: %s', request_url, response.status_code) -logger.info('Response for get request for url: %s, %s', url, response.text) +logger.info('Response for get request for url: %s, status: %s', request_url, response.status_code)Also applies to: 174-174
240-240: Enhance logging security and consistency in POST requestsWhile the addition of logging is beneficial, consider the following security and consistency improvements:
On line 240, avoid logging the full payload as it may contain sensitive information. Instead, log only necessary details or mask sensitive data.
On line 243, avoid logging the full response text for the same reason.
On line 246, maintain consistency with error logging in other methods.
Consider applying these changes:
-logger.debug('Payload for post request: %s', data) +logger.debug('Sending POST request to: %s', request_url) -logger.debug('Response for post request: %s', response.text) +logger.debug('Response for POST request to %s, status: %s', request_url, response.status_code) -logger.info('Response for post request: %s', response.text) +logger.info('Error response for POST request to %s, status: %s', request_url, response.status_code)Also applies to: 243-243, 246-246
Line range hint
1-258: Consider adding logging configuration optionsThe addition of logging across all methods in the Client class is a great improvement for traceability and debugging. However, to make the logging more flexible for users of the SDK, consider adding configuration options.
Implement a way for users to configure logging levels and handlers. This could be done by:
- Adding a method to the Client class to set the logging level:
def set_log_level(self, level): logger.setLevel(level)
- Providing documentation on how users can configure their own logging handlers if they want more control over log output.
This will allow users to adjust the verbosity of logs based on their needs, from silencing all logs to capturing detailed debug information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sage_desktop_sdk/core/client.py (7 hunks)
🧰 Additional context used
🔇 Additional comments (2)
sage_desktop_sdk/core/client.py (2)
2-2: LGTM: Logging setup looks goodThe addition of the
loggingimport and the logger initialization usinglogging.getLogger(__name__)follows best practices for setting up logging in Python modules.Also applies to: 18-18
206-206: Apply consistent logging improvementsThe logging changes in this method are identical to those in the
_query_get_allmethod. Please apply the same improvements suggested in the previous comment to these lines as well.Also applies to: 210-210
https://app.clickup.com/1864988/v/l/6-901603904304-1
Summary by CodeRabbit
New Features
Bug Fixes