[No QA][Sentry] Send App logs to Sentry#76335
Conversation
Logs attached to error |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| function mapLogMessageToSentryLevel(message: string): SentryLogLevel { | ||
| if (message.startsWith('[alrt]')) { | ||
| return 'error'; | ||
| } | ||
| if (message.startsWith('[warn]') || message.startsWith('[hmmm]')) { | ||
| return 'warn'; | ||
| } | ||
| if (message.startsWith('[info]')) { | ||
| return 'info'; | ||
| } | ||
| return 'debug'; | ||
| } |
There was a problem hiding this comment.
We're connecting Sentry logging to server log event so the input is string here. To decide what severity of logs should be logged I parse the log entry. Maybe you see better place to put forwarding so we can have all info without parsing the string?
|
@rlinoz Could you run adhoc build here? I want to check if CI would pass with changes to Sentry's config |
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
sosek108
changed the title
sosek108
changed the title
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
melvin-bot
bot
requested review from
JmillsExpensify and
truph01
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2025-12-04.at.16.36.43.mov |
| } | ||
|
|
||
| if (logLine.parameters) { | ||
| logMethod(logLine.message, logLine.parameters); |
There was a problem hiding this comment.
That is a good point, although I was thinking this would follow something along the lines of what we have in Auth, where we just redact parameters that are not whitelisted.
So we would do something related to that in a follow up PR where we fill the shouldForwardLog function.
There was a problem hiding this comment.
where we just redact parameters that are not whitelisted.
I think this is good idea.
in web we do have reduction logic before the logs are processed
only web? what's about mobile? Maybe we could move this log forwarding after data is redacted via expensify's mechanisms?
There was a problem hiding this comment.
only web? what's about mobile? Maybe we could move this log forwarding after data is redacted via expensify's mechanisms?
Mobile sends to Web-E and then Web-E actually logs it, so I think this would be cumbersome, it would go something like
- Call
Logcommand - Wait for a response
- Send the log to Sentry
This would probably make logs not follow the correct timeline I think.
There was a problem hiding this comment.
Ok, so we'll do redaction in follow up PR.
If you're ok with: we just redact parameters that are not whitelisted. I will do so
There was a problem hiding this comment.
Hmm but if we add redaction later there will be a version of the app with no redaction of the logs, not sure if I am following. We should make sure we redact them from beginning as we dont want to be dealing with removing sensitive data from Sentry so early 😅
There was a problem hiding this comment.
My initial line of thought was that:
If we don't send anything there is nothing to redact and redaction should be added alongside with logs that we add to sending.
But as I understand this thinking is shady we can copy the redaction from web (mentioned here) and then push this PR to merge.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|
6 participants
Explanation of Change
This PR introduces
forwardLogToSentrymethod which is responsible for forwarding logs to telemetry.Fixed Issues
$ #73322
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Nagranie.z.ekranu.2025-12-4.o.08.40.12.mov
Android: mWeb Chrome
iOS: Native
Nagranie.z.ekranu.2025-12-4.o.08.36.40.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2025-12-4.o.08.41.20.mov