feat: enhance logging for proxy testing errors #1790
Merged
+12
−6
Conversation
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
Reviewer's GuideThis PR refactors the testProxy method to improve observability by logging the outcome of the proxy test (success or unchanged origin IP) and enhancing error handling to capture specific Axios error messages and unexpected errors. Class diagram for updated ProxyController.testProxy methodclassDiagram
class ProxyController {
+testProxy(proxy)
}
class logger {
+info(message)
+warn(message)
+error(message)
}
class axios {
+isAxiosError(error)
}
ProxyController --> logger : uses
ProxyController --> axios : uses
Flow diagram for enhanced logging in testProxy methodflowchart TD
A[Start testProxy] --> B[Make request with Axios]
B -->|Success| C{Is response.data different from serverIp.data?}
C -- Yes --> D[Log info: proxy connection successful]
C -- No --> E[Log warn: proxy connection doesn't change the origin IP]
D --> F[Return result]
E --> F
B -->|Error| G{Is AxiosError?}
G -- Yes --> H[Log error: axios error + error.message]
G -- No --> I[Log error: unexpected error + error]
H --> J[Return false]
I --> J
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @henrybarreto - I've reviewed your changes - here's some feedback:
- In the unexpected error branch, pass the error object as a separate argument to logger.error (e.g. logger.error('testProxy error: unexpected error', error)) or JSON.stringify it to capture the full stack trace instead of string concatenation.
- Add a separator (e.g. ': ') before concatenating the error to improve log readability (currently it produces 'unexpected error[object Object]').
- The extra error.message check in the second branch is redundant since all AxiosError instances include a message—consider simplifying that condition to just axios.isAxiosError(error).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the unexpected error branch, pass the error object as a separate argument to logger.error (e.g. logger.error('testProxy error: unexpected error', error)) or JSON.stringify it to capture the full stack trace instead of string concatenation.
- Add a separator (e.g. ': ') before concatenating the error to improve log readability (currently it produces 'unexpected error[object Object]').
- The extra error.message check in the second branch is redundant since all AxiosError instances include a message—consider simplifying that condition to just axios.isAxiosError(error).
## Individual Comments
### Comment 1
<location> `src/api/controllers/proxy.controller.ts:63` </location>
<code_context>
+ logger.error('testProxy error: ' + error.message);
} else {
- logger.error('testProxy error: ');
+ logger.error('testProxy error: unexpected error' + error);
}
return false;
</code_context>
<issue_to_address>
String concatenation with an Error object may not yield useful output.
Instead of concatenating the error object, pass it as a separate argument to logger.error to retain full error details.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } else { | ||
| logger.error('testProxy error: '); | ||
| logger.error('testProxy error: unexpected error' + error); |
There was a problem hiding this comment.
issue: String concatenation with an Error object may not yield useful output.
Instead of concatenating the error object, pass it as a separate argument to logger.error to retain full error details.
4b20bcd to
a7600fe
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @henrybarreto - I've reviewed your changes and found some issues that need to be addressed.
- Consider logging the error.stack alongside the message to provide better debugging context when tracing issues.
- Rather than JSON.stringify(error), pass the error object directly to the logger or use util.inspect to capture non-enumerable properties and avoid missing details.
- Verify that logging error.response.data won’t expose any sensitive information—scrub or truncate if necessary before writing to the logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider logging the error.stack alongside the message to provide better debugging context when tracing issues.
- Rather than JSON.stringify(error), pass the error object directly to the logger or use util.inspect to capture non-enumerable properties and avoid missing details.
- Verify that logging error.response.data won’t expose any sensitive information—scrub or truncate if necessary before writing to the logs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
650f37a to
ca4f0cd
Compare
There was a problem hiding this comment.
Hey @henrybarreto - I've reviewed your changes - here's some feedback:
- Consider including error.response.data alongside error.message when logging Axios errors so you don’t lose potentially important server response details.
- For unexpected errors, pass the actual error object into logger.error rather than concatenating it to a string to preserve the full stack trace and context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider including error.response.data alongside error.message when logging Axios errors so you don’t lose potentially important server response details.
- For unexpected errors, pass the actual error object into logger.error rather than concatenating it to a string to preserve the full stack trace and context.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This commit improves the logging in the testProxy method of the ProxyController class. Now, when an Axios error occurs, the specific error message will be logged if available. For unexpected errors, the error object is included for better insight. For reference, see the "message" field in the Axios documentation: [Axios Error Handling](https://axios-http.com/docs/handling_errors).
ca4f0cd to
ab9e0ed
Compare
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.
This commit improves the logging in the testProxy method of the ProxyController class. Now, when an Axios error occurs, the specific error message will be logged if available. For unexpected errors, the error object is included for better insight.
For reference, see the "message" field in the Axios documentation: Axios Error Handling.
Summary by Sourcery
Enhance logging in ProxyController.testProxy to include explicit info/warn messages for success and IP unchanged scenarios and to log detailed Axios and unexpected errors
Enhancements: