[No QA][Sentry] Forward logs around reauthenticate flow#83300
Conversation
melvin-bot
Bot
requested review from
JmillsExpensify and
truph01
and removed request for
a team
melvin-bot
Bot
requested review from
a team and
mountiny
and removed request for
a team
![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
https://github.com/Expensify/App/blob/42b28d01ccd6a7366fb03017e6e868069ae34265/src/libs/telemetry/integrations.ts#L1
This new empty integrations.ts file masks the existing src/libs/telemetry/integrations/index.ts and index.web.ts for imports like @libs/telemetry/integrations (used by telemetry setup and navigation). TypeScript/Metro resolve the file before the directory, so named imports now come from an empty module and fail (e.g., TS2306: .../integrations.ts is not a module), which breaks telemetry initialization/builds.
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
We should hold this PR until #80645 is merged. |
|
Can you complete the review please? |
| .catch(() => { | ||
| setIsAuthenticating(false); | ||
| Log.hmmm('Redirecting to Sign In because we failed to reauthenticate after multiple attempts', {error}); | ||
| Log.hmmm('[Reauthenticate] Redirecting to Sign In because we failed to reauthenticate after multiple attempts', {error}); |
There was a problem hiding this comment.
Since the [Reauthenticate] and [Sentry] prefixes are used in many places throughout the app, could we define a shared constant like:
const LOG_PREFIX = {
REAUTHENTICATE: '[Reauthenticate]',
SENTRY: '[Sentry]',
};
Then use it here (and elsewhere) like this:
Log.hmmm(
`${LOG_PREFIX.REAUTHENTICATE} Redirecting to Sign In because we failed to reauthenticate after multiple attempts`,
{error},
);
This would help ensure consistency and make it easier to update the prefixes in the future.
There was a problem hiding this comment.
I'm not fond of this idea but this is my opinion so we should go with project standards.
When I see error log in logs and I want to go stright to the code I just copy the log string and search codebase with it. When we have something like this ${LOG_PREFIX.REAUTHENTICATE} Redirecting to Sign In because we failed to reauthenticate after multiple attempts my operation is no longer "no brainer, copy, paste , search". I need to think about how to search logline.
[Sentry] logs are just in one file. Your logic is quite valid about [Reauthenticate] as there are plenty of these in separate files.
adding this LOG_PREFIX to forwardLogsToSentry would make an impression that this is the only way of forwarding logs and this is not exactly true.
|
I left two minor comments |
| if (log.message?.includes('[Reauthenticate]')) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Should we instead prefix those logs with [Sentry]?
There was a problem hiding this comment.
what is the logic behind that?
This logs is related to re-authentication, not sentry. So the prefix should reflect the function it debugs.
if we hate passing logs via prefix we can do this via something other. Eg. add parameter "forwardToSentry: true". This is replacement but I don't think this has any benefits
There was a problem hiding this comment.
I was under the assumption that [Sentry] was essentially the forwardToSentry param, but in the log line, but I am ok with things being this way anyway.
|
Hey @truph01 you have 1 item unchecked I think |
|
@rlinoz I’ve completed the checklist, but I’m not sure why the “PR Reviewer Checklist” issue is still showing. It looks like it hasn’t re-run. |
|
it worked now, thank you! |
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.3.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
6 participants
Explanation of Change
This PR adds logging forwarding to Sentry with all logs related to reauthentication flow.
Fixed Issues
$ #73322
PROPOSAL:
Tests
Reauthenticatelogs are present thereOffline 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 stepssectiontoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari