Skip to content
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

[HOLD for payment 2024-11-05] [$250] Upload logs from Swift code #42659

Closed
1 of 6 tasks
m-natarajan opened this issue May 27, 2024 · 64 comments
Closed
1 of 6 tasks

[HOLD for payment 2024-11-05] [$250] Upload logs from Swift code #42659

m-natarajan opened this issue May 27, 2024 · 64 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented May 27, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.75-1
Reproducible in staging?: Needs reproduction
Reproducible in production?: Needs reproduction
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @arosiclair
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716825560361569

Action Performed:

User B logged in iOS device and notifications enabled

  1. Go to staging.new.expensify.com
  2. Send a message from A to B

Expected Result:

The notification received with the user's avatar

Actual Result:

No avatar displays in the notification

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

  • reportID 1911951290129650
  • reportActionID 5005778300960326136

Relevant logs:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018463b08116cdfa88
  • Upwork Job ID: 1795181230839853056
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 102978733
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @kirillzyusko
@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels May 27, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

Copy link

melvin-bot bot commented May 27, 2024

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label May 27, 2024
@melvin-bot melvin-bot bot changed the title iOS Push Notifications sometimes have no avatar [$250] iOS Push Notifications sometimes have no avatar May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

Job added to Upwork: https://www.upwork.com/jobs/~018463b08116cdfa88

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

@arosiclair arosiclair self-assigned this May 28, 2024
@arosiclair arosiclair added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels May 28, 2024
Copy link

melvin-bot bot commented May 31, 2024

@puneetlath, @arosiclair, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label May 31, 2024
@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented May 31, 2024

seems to be an internal assigned issue. Seems we don't have a PR yet.

@melvin-bot melvin-bot bot removed the Overdue label May 31, 2024
@arosiclair
Copy link
Contributor

arosiclair commented May 31, 2024

I wasn't able to find the root issue for this when I looked into it though I'm pretty sure it was a transient issue in the NotificationService for iOS.

We do os_log in that lib for most errors but those are not sent to the backend. I think we should find a way to write and upload logs from swift code so we can debug this kind of issue in the future.

@melvin-bot melvin-bot bot added the Overdue label Jun 3, 2024
@arosiclair
Copy link
Contributor

No progress on this yet

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 3, 2024
@arosiclair
Copy link
Contributor

Focusing on a critical issue atm.

@melvin-bot melvin-bot bot removed the Overdue label Jun 6, 2024
@arosiclair arosiclair added Weekly KSv2 and removed Daily KSv2 labels Jun 6, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

@puneetlath @arosiclair @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Oct 3, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.43-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-10. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 3, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@kirillzyusko] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@hannojg
Copy link
Contributor

hannojg commented Oct 3, 2024

The PR has been reverted, being suspected to increase heating / CPU consumption: https://margelo.slack.com/archives/C01GTK53T8Q/p1727896624465699

@arosiclair arosiclair removed the Awaiting Payment Auto-added when associated PR is deployed to production label Oct 3, 2024
@arosiclair arosiclair changed the title [HOLD for payment 2024-10-10] [$250] Upload logs from Swift code [$250] Upload logs from Swift code Oct 3, 2024
@arosiclair
Copy link
Contributor

Any ideas on what might be the issue? We didn't change much in our App code for this. Is there anything suspicious like a possible infinite loop in the react-native-app-logs repo? @hannojg @kirillzyusko

@hannojg
Copy link
Contributor

hannojg commented Oct 3, 2024

Kiryl is OOO and back Monday next week to have a detailed look!

@kirillzyusko
Copy link
Contributor

@arosiclair probably OSLogStore requires a lot of CPU resources so all operations are very expensive. I'll try to profile performance with XCode profiler and provide more details this week!

Anyway I have some potential optimizations in my head that we can apply and significantly reduce overheating 👀

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Oct 18, 2024
@kirillzyusko
Copy link
Contributor

@arosiclair I fixed overheating issue - would you mind to review and test new PR revision? #51086

@arosiclair
Copy link
Contributor

I'll take a look. What was the root problem?

@kirillzyusko
Copy link
Contributor

I'll take a look. What was the root problem?

The operation with OSLogStore (logs retrieving) is a pretty complex operation in terms of performance and on a real devices takes much more time than on simulator (on my iPhone 11 it was taking 1s). Since we did it every 5s the device got overheated pretty quickly.

I re-worked code and made interval customizable. Since we want to intercept only push-notifications logs I disabled constant pulling from main app at all. I think such approach will significantly free up resources.

However I don't know whether device will be overheated if you receive messages (i. e. notifications) frequently - that's something that would be good to test 🙌

@arosiclair
Copy link
Contributor

Cool, nice work 👍. I'll review soon-ish. I have some higher priority projects to address atm though.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 29, 2024
@melvin-bot melvin-bot bot changed the title [$250] Upload logs from Swift code [HOLD for payment 2024-11-05] [$250] Upload logs from Swift code Oct 29, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 29, 2024
Copy link

melvin-bot bot commented Oct 29, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 29, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.54-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-05. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 29, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@kirillzyusko] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@arosiclair
Copy link
Contributor

No regression tests needed since this is just for logging. Thanks for the help @kirillzyusko! Closing this out.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests