-
Notifications
You must be signed in to change notification settings - Fork 1
Bugfix: Add debug option to log response headers #219
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
base: v1.2
Are you sure you want to change the base?
Conversation
|
|
||
| private debug(context: ResponsePluginContext) { | ||
| if (context.requestOptions.logHeadersOnError && !context.response?.ok) { | ||
| console.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about supporting also for this SDK a logger interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added logger
| context.response?.headers, | ||
| if (!context.response?.ok) { | ||
| this.logger.debug( | ||
| `[Sinch SDK][Debug][${context.apiName}][${context.operationId}][${context.response?.status}]\nHTTP method: ${context.requestOptions.method}\nURL: ${context.url}\nResponse Headers: ${this.formatHeaders(context.response?.headers)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having the logger itself adding these formatting information ?
- this.logger: will know "it is" SDK -> it will add it when emitting log
- this.logger.debug: logger.debug function will know it is debug level then it will be added when emitting log
PROS:
- all calls to
loggerwill be prefixed by same ... prefix - all calls to
logger./debug/info/..will have same prefix regarding log level - all logs will have same pattern [Sinch SDK][level] and no human error to forgot them services will call logger function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
May be adding some unit tests to ensure proper/format and log level.
Not sure we want to go to this direction for console logger and this PR, but other logging frameworks (e.g.: winston pointed by PR comment) are enabling to configure log level to not flood logs output/destination
How to enable debug logs:
Debug logs: