fix(plugin-meetings): improved logging#5011
Conversation
marcin-bazyl
added
the
validated
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80bcc08b45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| options.logger.warn( | ||
| `http ${options.method ? options.method : 'request'} to ${options.uri} result: ${ | ||
| response.statusCode | ||
| }` | ||
| } body: ${JSON.stringify(response.body)}` | ||
| ); |
There was a problem hiding this comment.
In browser builds, every 4xx/5xx response now writes the full response body at warn level, even when ENABLE_VERBOSE_NETWORK_LOGGING is disabled. For requests to auth or other endpoints whose error payloads can include user-provided content or sensitive details, this bypasses the existing response logger's verbose-only behavior and can persist those payloads in the configured SDK logs; please keep body logging behind the verbose flag or redact/limit it before logging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
i tend to agree with codex here ^
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
chrisadubois
left a comment
chrisadubois
left a comment
There was a problem hiding this comment.
- running JSON.stringify could throw
- I kind of agree with codex re: auth logs. I don't think it needs to be behind verbose, but i think we want to be careful about logging PII
| options.logger.warn( | ||
| `http ${options.method ? options.method : 'request'} to ${options.uri} result: ${ | ||
| response.statusCode | ||
| }` | ||
| } body: ${JSON.stringify(response.body)}` | ||
| ); |
There was a problem hiding this comment.
i tend to agree with codex here ^
2 participants
This pull request addresses
When some network request fails and the error is not caught anywhere, we have a log that shows the status code but not the body. The body usually contains more specific backend error message and error code, very useful for debugging
by making the following changes
Modified log to show also the response body. Error responses should always be quite short and small, so I think it should be fine to just log the whole response.
Also improved some logs in LocusInfo based on recent experience when debugging various issues.
Change Type
The following scenarios were tested
unit tests
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.