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 2022-08-11] [$250] Electron log file not working #9794

Closed
roryabraham opened this issue Jul 8, 2022 · 25 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

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


Action Performed:

Run a console.log in the Electron application.

Expected Result:

The log should be added to a log file for the app located at ~/Library/Logs/new.expensify/main.log

Actual Result:

The latest log in my ~/Library/Logs/new.expensify/main.log file is from 4 months ago.

Workaround:

n/a

Platform:

Where is this issue occurring?

  • Desktop App

Version Number: 1.1.82-3
Reproducible in staging?: yes
Reproducible in production?: yes

View all open jobs on GitHub

@roryabraham roryabraham added External Added to denote the issue can be worked on by a contributor Engineering Weekly KSv2 Improvement Item broken or needs improvement. labels Jul 8, 2022
@roryabraham roryabraham self-assigned this Jul 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 8, 2022
@roryabraham
Copy link
Contributor Author

Some information about the logs here. I know this used to work, and it was probably broken back when I set up a secure Electron implementation with IPC and nodeIntegration disabled: #7567.

@roryabraham
Copy link
Contributor Author

The latest log I have in my log file is:

[2022-03-03 18:51:25.729] [info]  Update for version 1.1.40-2 is not available (latest version: 1.1.40-2, downgrade is disallowed).

So that makes me guess that electron-log was broken in the following release: https://github.com/Expensify/App/releases/tag/1.1.41-6

@roryabraham
Copy link
Contributor Author

Interesting that release was the one that set up the 2-package structure. see the full diff here: 1.1.40-2...1.1.41-6

cc @kidroca

@MitchExpensify
Copy link
Contributor

Upwork Job

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 8, 2022
@MitchExpensify MitchExpensify added the Daily KSv2 label Jul 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

Current assignee @roryabraham is eligible for the Exported assigner, not assigning anyone new.

@MitchExpensify MitchExpensify removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 8, 2022
@melvin-bot melvin-bot bot changed the title Electron log file not working [$250] Electron log file not working Jul 8, 2022
@kidroca
Copy link
Contributor

kidroca commented Jul 11, 2022

@roryabraham when I worked on the "2-package" structure I needed to check logs, but couldn't find anything under ~/Library/Logs/new.expensify/main.log

I assumed I was probably doing something incorrectly because I could find no logs there

But now that I saw this issue, it might be due to console.log statements being dropped from the main.js code, since it's bundled by webpack (with mode: production) and this would remove console.log statempets

We should probably override that for Desktop main if we want console.log statements to remain

@melvin-bot melvin-bot bot added the Overdue label Jul 18, 2022
@roryabraham
Copy link
Contributor Author

@kidroca let me know if you want this assigned to you. Otherwise @MitchExpensify let's double this

@melvin-bot melvin-bot bot removed the Overdue label Jul 18, 2022
@kidroca
Copy link
Contributor

kidroca commented Jul 18, 2022

@roryabraham

@kidroca let me know if you want this assigned to you. Otherwise @MitchExpensify let's double this

Yes, sounds good, I can post a small webpack config patch to preserve console statements for the main electron process

@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

📣 @kidroca You have been assigned to this job by @roryabraham!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@roryabraham
Copy link
Contributor Author

@mananjadhav Going to unassign you on this one because it sounds like @kidroca's got this. The fix should be pretty easy to validate.

@kidroca
Copy link
Contributor

kidroca commented Jul 19, 2022

I think I'll be ready with a PR tomorrow or the day after

@roryabraham
Copy link
Contributor Author

Cool, thanks @kidroca. Full-disclosure: I'm pretty swamped this week so it might take me a few days to get to the PR. I'll do my best to review it as soon as I can

@MitchExpensify MitchExpensify changed the title [$250] Electron log file not working [$500] Electron log file not working Jul 19, 2022
@MitchExpensify
Copy link
Contributor

Doubled! Upwork job

@roryabraham
Copy link
Contributor Author

Thanks @MitchExpensify – I think since @kidroca is working on this he'll just bill us hourly. Feel free to correct me if I'm wrong @kidroca

@kidroca
Copy link
Contributor

kidroca commented Jul 20, 2022

Thanks @MitchExpensify – I think since @kidroca is working on this he'll just bill us hourly. Feel free to correct me if I'm wrong @kidroca

Yes, I'll add the work here under the hourly contract, it's more convenient

(💲 Don't tempt me by doubling the price 💲)

@MitchExpensify
Copy link
Contributor

Thanks for the catch @roryabraham! Sounds good @kidroca :D

@MitchExpensify MitchExpensify changed the title [$500] Electron log file not working [$250] Electron log file not working Jul 20, 2022
@kidroca
Copy link
Contributor

kidroca commented Jul 25, 2022

It turned out console logs are not stripped from the minified main.js code

I've added a bunch console statements
Then built the desktop app
And the console statements can all be seen in desktop/dist/main.js - the bundled code

I've searched a bit more and found the log file in: ~/Library/Logs/new.expensify.desktop
It contained everything that has been logged and my sample logs
The new folder name is the result of the desktop/package.json

"name": "new.expensify.desktop",

Here's how electron-log picks the log file location: https://github.com/megahertz/electron-log/blob/HEAD/docs/file.md#appname-string-deprecated

Determines a location of log file, something like ~/.config/<app name>/log.log depending on OS. By default, electron-log reads this value from name or productName value in package.json. In most cases you should keep default value.

  1. Can you verify you have newer logs in - ~/Library/Logs/new.expensify.desktop (vs ~/Library/Logs/new.expensify)
  2. Do we want to override that location or set a different name for the package?

@roryabraham
Copy link
Contributor Author

Can you verify you have newer logs in - ~/Library/Logs/new.expensify.desktop (vs ~/Library/Logs/new.expensify)

Yes! I've got them there

Do we want to override that location or set a different name for the package?

Nope, let's just update the documentation with the new location here and here and maybe add something to the README in the desktop folder about the log file. Thanks!

@kidroca
Copy link
Contributor

kidroca commented Jul 27, 2022

@kidroca
Copy link
Contributor

kidroca commented Jul 27, 2022

BTW I've found out some additional stuff that we can do

  • we can prefix auto update related logs with autoUpdater
  • we can capture renderer logs to file if we register electron-log in the context bridge
  • we can output main logs in the dev tools console (be able to see the autoUpdater logs live for example)

Any interest in any of these?

@roryabraham
Copy link
Contributor Author

roryabraham commented Jul 27, 2022

Sure @kidroca, happy to review PRs for all of those things. Thanks!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 4, 2022
@melvin-bot melvin-bot bot changed the title [$250] Electron log file not working [HOLD for payment 2022-08-11] [$250] Electron log file not working Aug 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.87-9 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 2022-08-11. 🎊

@MitchExpensify
Copy link
Contributor

Thanks for you help with this @kidroca ! I see you've added it to your hourly contract so we're all good here

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 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

4 participants