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-05-16] ][HIGH] [Debugability] [$500] Save client-side logs and profile traces to the Downloads folder #40213

Closed
quinthar opened this issue Apr 13, 2024 · 53 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor High Priority NewFeature Something to build that is a new item. Task

Comments

@quinthar
Copy link
Contributor

quinthar commented Apr 13, 2024

Problem:

We have a great system in place to enable client-side logging and profile tracing with a four-finger tap. This allows for really fantastic debugging on production devices out in the real world. However, the files are saved in a private folder that -- so far as I can tell -- is impossible for a user on a non-rooted device to access. So after getting the logs and profile traces, if they don't share them at that exact moment, there's no way to get them in the future.

Solution:

Save client side logging and profile trace files to the /Downloads folder on Android (and whatever similarly accessible folder is generally accessible on iOS.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0143be8cb9f6347710
  • Upwork Job ID: 1779286276762456064
  • Last Price Increase: 2024-05-03
  • Automatic offers:
    • ishpaul777 | Reviewer | 0
    • ShridharGoel | Contributor | 0
@quinthar quinthar converted this from a draft issue Apr 13, 2024
@quinthar quinthar added External Added to denote the issue can be worked on by a contributor Daily KSv2 NewFeature Something to build that is a new item. Task labels Apr 13, 2024
Copy link

melvin-bot bot commented Apr 13, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0143be8cb9f6347710

@melvin-bot melvin-bot bot changed the title HIGH: [Reliability] Save client-side logs and profile traces to the Downloads folder [$250] HIGH: [Reliability] Save client-side logs and profile traces to the Downloads folder Apr 13, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 13, 2024
Copy link

melvin-bot bot commented Apr 13, 2024

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

Copy link

melvin-bot bot commented Apr 13, 2024

Triggered auto assignment to @stephanieelliott (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 13, 2024
@quinthar quinthar changed the title [$250] HIGH: [Reliability] Save client-side logs and profile traces to the Downloads folder HIGH: [Reliability] [$250] Save client-side logs and profile traces to the Downloads folder Apr 13, 2024
@allgandalf
Copy link
Contributor

Proposal

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

We need to save logs in downloads folder on android

What is the root cause of that problem?

Feature requests

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

We need to update the native files below:

const localFileDownload: LocalFileDownload = (fileName, textContent, successMessage) => {

In here, we can use the function RNFetchBlob.fs.dirs.DownloadDir to first get the path of the download directory on android and then later use RNFetchBlob.fs.cp(path, destinationPath) to download the file on android device, the updated code would look like:

RNFetchBlob.fs.exists(RNFetchBlob.fs.dirs.DownloadDir)
            .then(() => {
                i
                    const destinationPath = `${RNFetchBlob.fs.dirs.DownloadDir}/${newFileName}`;
                    RNFetchBlob.fs.cp(path, destinationPath)
                        .then(() => {

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2024
@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 14, 2024

Proposal

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

Show meaningful path of saved logs.

What is the root cause of that problem?

New change.

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

Since files are already there in public folders, we can just show a meaningful path.

Instead of using file.path, just show /Downloads/${file.newFileName} for Android and /New Expensify Dev/${file.newFileName} for iOS.

<Text style={[styles.textLabelSupporting, styles.mb4]}>{`path: ${file.path}`}</Text>

Same changes are needed for profiling path.

<Text style={[styles.textLabelSupporting, styles.mb4]}>{`path: ${pathIOS}`}</Text>

Also, change the newFilePath to use DownloadDirectoryPath for Android. For iOS, we can use the existing DocumentDirectoryPath.

@dominictb
Copy link
Contributor

dominictb commented Apr 14, 2024

Proposal

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

We want to save client side logging and profile trace files to the /Downloads folder on Android (and whatever similarly accessible folder is generally accessible on iOS.

What is the root cause of that problem?

  1. For Android, we're already saving to Downloads folder (can be seen here), but the file path that we're showing is of the localFile, which might seem misleading to users because they might think the file hasn't been saved to Downloads.
  2. For iOS, we're not saving the file to Downloads, but only have a local file, can be seen here

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

  1. We should show the Downloads file path instead of the localFile path (the one that's in private folder as discouraged in the OP).

In here, set the path returned after saving the file to Downloads to a mediaPath field:

.then(path => {
    setFile({
        ...localFile,
        mediaPath: path,
    });
});

Then in here, use the file.mediaPath to show in path: , if file.mediaPath does not exist then fallback to file.path

(Since the device-download path looks cryptic, we maybe should instead show to the users the readable file name instead so they know how to access the file, something like path: /Downloads/logs-2024-04-14 10_58_29.432.txt, this can easily be done by using the newFileName)

  1. We should start to save the file to Downloads (or just to Files), similar to what we did with Android, and also use the same mediaPath approach too.

What alternative solutions did you explore? (Optional)

NA

@brkdavis
Copy link

Proposal

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

New feature to save app logs to Downloads instead of default app private folder.

What is the root cause of that problem?

This is a new feature.

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

Before suggesting my solution, @quinthar / @ishpaul777 I would like to bring 2 points to think about and consider before proceeding with the solution.

  1. Even if we save the logs to private folder or to Downloads, the logs are still local to the device and we are at the mercy of the end user to upload or share them with us for Expensify team to support in any debugging.
  2. Local logs in silos are still a huge amount of valuable data that is scattered in individual devices. They do not add any value to us until it reaches us.

I propose to have some or all of these logs centralized and collected to one of the cloud service provider data stores location so that the same can be used for big data analytics and even for supporting in troubleshooting purposes. This must give a great insight into how the user is using the application and will also give insight into how we can improve our application and its user interface to provide better service to the user. Of course, this essentially means that we must get user consent (maybe as an optional service) at the time of installation and have these uploaded to data stores at regular intervals.
Points to consider:

  • Chat data must be end-to-end encrypted, if these are also in the logs. Or these logs must be omitted from uploading.
  • Privacy is at most priority. Any sensitive and personally identifiable information must be masked or anonymized .
  • Automatic uploading must be done only for analytical and app usage data.
  • In case when a user raises a support ticket, they can be provided an option to share logs for troubleshooting purposes and then we will already have all the data already available to the support person from the cloud server. Any logs that are not opted in for automatic upload must be uploaded automatically from device private folder itself to the cloud server.

Having a log available in Downloads folder means that the same can be accessed or read by others or by other applications. Having the logs saved into the app private folder structure is the best solution according to me.

@quinthar / @ishpaul777 : If my above thoughts make sense, I would like to work on this from backend as well as front end (from a full stack capacity) to architect, design and implement the solution.

What alternative solutions did you explore? (Optional)

N.A.

@quinthar
Copy link
Contributor Author

@brkdavis - Good questions! We already centrally aggregate these logs. They are just inconvenient to access, and only by a limited set of people (given all the concerns you raised). The point of local log capture is to make someone's own logs much easier for them to access. By capturing local logs into the Download folder, we make someone's own logs easier for them to access for whatever purpose they want. So, good questions, but I still would like to go ahead with this issue as stated.

@brkdavis
Copy link

brkdavis commented Apr 14, 2024

@brkdavis - Good questions! We already centrally aggregate these logs. They are just inconvenient to access, and only by a limited set of people (given all the concerns you raised). The point of local log capture is to make someone's own logs much easier for them to access. By capturing local logs into the Download folder, we make someone's own logs easier for them to access for whatever purpose they want. So, good questions, but I still would like to go ahead with this issue as stated.

@quinthar : thanks a lot for the clarifications. Couple of questions in direction of logs in Downloads folder.

  • Do we have a log viewer within the app?
  • Should the logs automatically get logged to Downloads folder ss and when the log entries and generated? Or is it only upon 4 finger tap we must export the logs from private folder to Downloads so that user can share from there at any point in time? I believe it is the latter. Kindly confirm.

@quinthar
Copy link
Contributor Author

quinthar commented Apr 16, 2024

Do we have a log viewer within the app?

Yes, in About > Troubleshooting > View console logs

Should the logs automatically get logged to Downloads folder ss and when the log entries and generated? Or is it only upon 4 finger tap we must export the logs from private folder to Downloads so that user can share from there at any point in time? I believe it is the latter. Kindly confirm.

This issue is just about changing where it's logged, not when. So it's currently logged when you enable via 4-finger tap, or via About > Troubleshooting. So let's keep it working as is, just change where it saves the logs to.

@quinthar
Copy link
Contributor Author

What are the next steps here; are we waiting on @ishpaul777 to pick a proposal? What's the ETA?

@stephanieelliott
Copy link
Contributor

Hey @ishpaul777 we have a couple proposals here -- can you please review ASAP?

@ishpaul777
Copy link
Contributor

Hey Thanks for bump i have this planned for today : )

@ishpaul777
Copy link
Contributor

Started android build to test existing proposals, i'll test and update in a while

@ishpaul777
Copy link
Contributor

ishpaul777 commented Apr 17, 2024

Thanks for your proposals everyone!

For Android, we're already saving to Downloads folder (can be seen here), but the file path that we're showing is of the localFile, which might seem misleading to users because they might think the file hasn't been saved to Downloads. -> from @dominictb Proposal

This seems correct we are just not showing the correct path but we do save logs in /downloads folder but not the profile traces as we can see here

const path = await stopProfiling(getPlatform() === CONST.PLATFORM.IOS);

so first we need to fix it to save in downloads then we need to fix path shown for profile trace to downloads path

I'd like to see a proposal that fixes this

For iOS, we're not saving the file to Downloads, but only have a local file, can be seen here

i dont this we need fix the destination for ios, i think we already save it in a accessible folder i.e On my iphone -> New expensify, @quinthar Do you think we need to change this one?

Also as part of proposal please include how you plan to clean the path shown to user as most time its cryptic

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Apr 23, 2024
@ShridharGoel
Copy link
Contributor

#40777

@quinthar quinthar changed the title HIGH: [Reliability] [$250] Save client-side logs and profile traces to the Downloads folder HIGH: [Debugability] [$250] Save client-side logs and profile traces to the Downloads folder Apr 27, 2024
@stephanieelliott
Copy link
Contributor

I'm back from OOO, thanks for watching this while I was out @abekkala!

The PR is awaiting approving review and all conflicts have been resolved. Based on this it seems we're now in the process of running an adhoc build

@abekkala abekkala removed their assignment May 2, 2024
@quinthar
Copy link
Contributor Author

quinthar commented May 2, 2024

Great, we're 3 weeks into this issue, so super eager to get it done.

@ShridharGoel
Copy link
Contributor

@stephanieelliott This is marked as HIGH but it is missing the high priority label. Can you check it?

@quinthar
Copy link
Contributor Author

quinthar commented May 3, 2024

What are the next steps here, who is doing them, and what is the ETA? This feels like a one-line change, can you help me understand why it's taking so long?

@ishpaul777
Copy link
Contributor

ishpaul777 commented May 3, 2024

Hey 👋 the Pr is merged for this, we'll have it on staging after next depoloy

@mallenexpensify mallenexpensify changed the title HIGH: [Debugability] [$250] Save client-side logs and profile traces to the Downloads folder [HIGH][Debugability] [$250] Save client-side logs and profile traces to the Downloads folder May 3, 2024
@stephanieelliott
Copy link
Contributor

stephanieelliott commented May 3, 2024

@stephanieelliott This is marked as HIGH but it is missing the high priority label. Can you check it?

Sure @ShridharGoel, we can add the High Priority label, seems like the automation was broken

@stephanieelliott stephanieelliott changed the title [HIGH][Debugability] [$250] Save client-side logs and profile traces to the Downloads folder [HIGH][Debugability] [$500] Save client-side logs and profile traces to the Downloads folder May 3, 2024
Copy link

melvin-bot bot commented May 3, 2024

Upwork job price has been updated to $500

@stephanieelliott
Copy link
Contributor

This was deployed to prod earlier today

@stephanieelliott stephanieelliott changed the title [HIGH][Debugability] [$500] Save client-side logs and profile traces to the Downloads folder [HOLD for payment 2024-05-16] ][HIGH] [Debugability] [$500] Save client-side logs and profile traces to the Downloads folder May 9, 2024
@stephanieelliott stephanieelliott added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels May 9, 2024
@quinthar
Copy link
Contributor Author

Woohoo, this is great!!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 15, 2024
@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

Heads up since this job was already set at $250, the payment was issued as $250 milestone payment + $250 bonus for a total of $500.

Upwork job is here: https://www.upwork.com/jobs/~0143be8cb9f6347710

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 Daily KSv2 External Added to denote the issue can be worked on by a contributor High Priority NewFeature Something to build that is a new item. Task
Projects
No open projects
Development

No branches or pull requests

9 participants