Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.37k stars 2.79k forks source link

[$250] Upload logs from Swift code #42659

Open m-natarajan opened 4 months ago

m-natarajan commented 4 months ago

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
    • [x] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

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: @arosiclair
MelvinBot commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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.

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

abdulrahuman5196 commented 4 months ago

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

arosiclair commented 4 months ago

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.

arosiclair commented 4 months ago

No progress on this yet

arosiclair commented 3 months ago

Focusing on a critical issue atm.

melvin-bot[bot] commented 3 months ago

@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!

arosiclair commented 3 months ago

Since the plan is to just improve logging for swift code, I'm going to switch this to a NewFeature and mark this External.

melvin-bot[bot] commented 3 months ago

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 3 months ago

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 3 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

arosiclair commented 3 months ago

Problem We have some native iOS swift code that configures notifications here. This code logs errors using os_log but these are local only so it's not possible to debug those issues in production like the one reported in the OP.

Solution Find some way to upload logs from Swift code to our server like we do in React here so that we can debug issues in native code.

arosiclair commented 3 months ago

@dubielzyk-expensify this is just a logging improvement so no design input needed here.

melvin-bot[bot] commented 3 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

b4s36t4 commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Upload logs from Swift code

What is the root cause of that problem?

No Problem, it's a feature request.

What changes do you think we should make in order to solve the problem?

To support reading logs from Swift code we need to add native module to our app.

The implementation that I'm trying doesn't support devices with iOS version less than 15.0 which I think doesn't seem to be a worry for us.

https://gs.statcounter.com/ios-version-market-share/ Seems nobody now using devices with less v15.0 which is a great thing.

The support to read logs from device is done by using OsLogStore class from osLog class provided in swift.

using OsLogStore.getAllEntries will give the all logs happened with the iOS devices (or current process) based on that we can process the log message.

I'm attaching a sample function that I have tested with and working fine.

  @objc(withResolver:withRejecter:)
  func logs(resolve:RCTPromiseResolveBlock,reject:RCTPromiseRejectBlock) -> Void {
    if #available(iOS 15.0, *) {
     let subsystem = Bundle.main.bundleIdentifier!
     // Open the log store.
     do {
     let logStore = try OSLogStore(scope: .currentProcessIdentifier)

     // Get all the logs from the last hour.
     let oneHourAgo = logStore.position(date: Date().addingTimeInterval(-3600))

     // Fetch log objects.
     let allEntries = try logStore.getEntries(at: oneHourAgo)

     // Filter the log to be relevant for our specific subsystem
     // and remove other elements (signposts, etc).
     let bundleLogs = allEntries
         .compactMap { $0 as? OSLogEntryLog }
         .filter { $0.subsystem == subsystem }
         .map { $0.formatString }

      resolve(bundleLogs);
     }catch {
      reject("OSVersionError","Error", nil)
     }
    }else{
      reject("OSVersionError","Not possible", nil)
    }
  }

With the above code we can read the logs and share it with react-native.

We can create a new module and attach this to expensify app, whenever we want logs we can use logs() function exposed from swift inside our JS code and save it along with the existings logs.

What alternative solutions did you explore? (Optional)

NA

Demo

https://github.com/Expensify/App/assets/59088937/cc3b6434-8a2e-4c38-9e46-e35d977423e9

The above demo is showing that inside multiply function I'm doing logs 4 times which I can read later after calling the multiply function all throughout using JS with native swift code.

Update

We can also capture the level of logging i.e either Error or Info or so using the following sinppet changes

 let bundleLogs = allEntries
         .compactMap { $0 as? OSLogEntryLog }
         .filter { $0.subsystem == subsystem }
         .map { "[\(logLevelStrings[$0.level.rawValue] ?? "Unknown")] \($0.formatString)" }

logLevelStrings is a map of available types of log levels with string values.

Attaching demo too

Screenshot 2024-06-13 at 3 49 48β€―AM
b4s36t4 commented 3 months ago

@abdulrahuman5196 @arosiclair Soft ping for proposal review, thanks!

arosiclair commented 3 months ago

Thanks for the proposal @b4s36t4. Is there a way we can actively push logs to our JS logging system rather than fetching logs from Swift after the fact? Put another way, is there a way to implement Log.info() Log.hmmm(), and etc. in Swift and have those logs pushed to JS directly?

b4s36t4 commented 3 months ago

No @arosiclair. Basically there's no way to stream the logs directly through code as far as I've researched.

This is the only I was able to extract the logs, I'll try if there's anyway to stream the logs directly.

arosiclair commented 3 months ago

there's no way to stream the logs directly through code as far as I've researched.

Do you mean logs written to os_log specifically? There's no requirement to keep using os_log if it prevents this. Is there a way we can communicate from Native to React Native to write logs?

melvin-bot[bot] commented 3 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

mvtglobally commented 3 months ago

Issue not reproducible during KI retests. (First week)

arosiclair commented 3 months ago

@mvtglobally no need to reproduce this issue

arosiclair commented 3 months ago

Looks like we may have some agencies look at this (thread)

chrispader commented 3 months ago

I started working on a proposal for this, but first i'd like get some opinions about how we go forward with writing native modules / TurboModules.

Posted a thread here: https://expensify.slack.com/archives/C01GTK53T8Q/p1719311680059169

melvin-bot[bot] commented 3 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

chrispader commented 3 months ago

@kirillzyusko is going to take this over, because i'm gonna be OOO for the next few weeks.

I did some initial research and shared my findings with @kirillzyusko. Also there was some good traffic in the slack thread, so we should be good to go with (potentially) implementing a native module to solve this problem

melvin-bot[bot] commented 3 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

kirillzyusko commented 3 months ago

@MelvinBot I'm working on this issue πŸ‘€

melvin-bot[bot] commented 3 months ago

πŸ“£ @abdulrahuman5196 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

arosiclair commented 2 months ago

Any updates on this @kirillzyusko?

kirillzyusko commented 2 months ago

@arosiclair yeah, I implemented a necessary changes (I created a separate library, because it was easier/faster).

The only one problem right now is to verify that os_log logs are actually can be retrieved. For that we need to test send/receive push notifications, and here is the problem, because I don't belong to Expensify team and I don't have certificates to sign the app.

@hannojg suggested the idea of testing it locally in example app - but here is also a problem, since he tried to invite me to his dev team, but dev team is not showing up in my XCode settings πŸ€·β€β™‚οΈ

I've also asked @Kureev to check the example app (he had some spare cycles, but as I know he didn't start to work on this task yet). And @Szymon20000 also offered a help (he belongs to Expensify dev team). So we are discussing it internally how to test that/etc. πŸ‘€

arosiclair commented 2 months ago

Sounds good thanks for the update. If you guys can't find a solution, I can try testing a draft PR if you have one (though I don't have much bandwidth atm).

hannojg commented 2 months ago

No worries, I have time today to look into this with Kiryl! 😊

kirillzyusko commented 2 months ago

@arosiclair I opened a PR https://github.com/Expensify/App/pull/45905

I decided to move all functionality with forwarding logs to separate library/module (react-native-app-logs), because it'll be easier to support code in such way.

Let me also quickly describe how it works - when NotificationService gets destroyed (i. e. we received notification) we pull all logs and forward them to main app (we need to forward them to main app, because NotificationService doesn't know anything about react-native). In iOS all processes are isolated, so to share the data between them they must belong to the same app group.

So to make this library work in Expensify we need to add new capability to the main app and NotificationService - App Group. And we need to use common identifier between these 2 groups (in my PR I used group.com.expensify.chat).

Once it's done we need to recompile the app, send push notification and verify that everything works πŸŽ‰ I tested everything in example app and it works pretty stable. But I can not accomplish remaining changes, because I'm not belonging to the Expensify app.

So @arosiclair may I ask you to help me to finish this task (i. e. to add AppGroup capability + setup common identifier and finally test that everything works)?

arosiclair commented 2 months ago

Excellent work I'll try making some time to give it a test. If everything looks good I'll get started on adding the new capability (very annoying process πŸ˜…).

arosiclair commented 1 month ago

No updates for this yet

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

arosiclair commented 1 month ago

Looks like we'll have to add the App Groups entitlement to our (NewApp) provisioning profiles before I can run the app and test. I'll open up an issue for it.

arosiclair commented 1 month ago

On second thought, maybe we should hold on setting this up considering it'll need to be re-done for the hybrid app anyway? @AndrewGable @Julesssss what do you think? We're working on pushing logs from native Swift code to our servers so we can debug issues with notification styling that happen in prod.

AndrewGable commented 1 month ago

I think we should continue to work on that, seems like a great improvement regardless of HybridApp. We have other tools to do this on OldApp, but we will depreciate them eventually.

arosiclair commented 4 weeks ago

@kirillzyusko we just merged changes to enable app groups! Can you update your PR to pull those changes and use group.com.expensify.new for the group?

kirillzyusko commented 4 weeks ago

Can you update your PR to pull those changes and use group.com.expensify.new for the group?

@arosiclair yep, updated the PR! Feel free to check it πŸ™Œ

Or @chrispader is going to check how it works?

chrispader commented 3 weeks ago

i can have a look at it. going to build it on a physical device (for context: @kirillzyusko isn't part of the provisioning profile to build on physical devices)

arosiclair commented 2 weeks ago

Attempted a test on this. Comments on the PR.

arosiclair commented 21 hours ago

No C+ review was needed since this was just an iOS change. PR is merged and on its way to deploy.