Enhancement Proposal: Logging

[HadesArchitect]

Problem

At the moment, the Workbench uses an in-house logging module. It violates best practices, introduces the need to maintain it on our own and makes the code base more complicated.

Suggestion

We can improve the project extensibility and maintainability by using a standard logging module. I suggest refactoring the Workbench using the Electron logging library electron-log

Rationale

• Removes the need to maintain and develop a custom logging solution
• Simplifies the workbench source code by removing redundant IPC logging communication
• Adds multiple features convenient features (multi-channel, log buffering etc., see below)
• Follows Best Practices for Electron Development
• Helps potential Open-Source contributors with onboarding (usage of a standard component instead of an in-house one)

Details

• PR to demonstrate suggested approach: Demo: electron-log #630
• electron-log NPM repository

Sample Code

main.js

 // Include
const log = require('electron-log/main')

// Initialize logging for renderers
log.initialize();

// Set proper verbosity levels for different channels
log.transports.console.level = 'debug'; 
log.transports.file.level = 'warn'

// Write logs
log.info('Greetings from main')
log.error('something went wrong')

init.js (renderer)

 // Include
const log = require('electron-log/renderer')

// Warning
log.warn('Required file doesn\'t exist', file, exception)

Output Samples

Of course, the output can be customized in any way we want.

Considered Alternatives

At the moment, electron-log is the only "electron-aware" logger I'm aware of; all the other solutions will require proper adaptation (e.g., injecting them into renderers and using IPC to deliver logs to the main process), which is definitely not the job we want to do. Theoretically beneficial could be the use of e.g. Winston, which is more powerful and configurable, but it's again not electron-aware, and I doubt we need it configurability.

electron-log Pros and Cons

🟢 De-facto standard solution (130 forks, 1.3 stars), known and maintained
🟢 Electron-aware logger (Native support of renderer processes, no need for direct IPC communication)
🟢 Out-of-the-Box Multi-channel output support (console + file + centralised log storage, if desired)
🟢 Simple replacement/extension for default console.log (no need for custom solution)
🟢 Out-of-the-box capture and logging of internal electron errors/warnings, etc.
🟢 Logging scopes support
🟢 Logs buffering (show minimum if all good, print all logs in case of failure)
🔴 Requires solid refactoring

Action Plan

If stakeholders approve the proposal, I will refactor the code and make a new pull request. If approval is granted quickly, it can be done before Christmas.

[HadesArchitect]

@millerjp @digiserg @mhmdkrmabd Dear team, I'd like to know your opinion on that. I'd like to give you more time, but resources are limited, and I'll have much less time in January.

[digiserg]

@HadesArchitect I have no issues, it sounds like a good proposal.

[millerjp]

@HadesArchitect Makes sens to me!

[mhmdkrmabd]

@HadesArchitect thank you for that enrich details! switching to electron-log sounds rational enough here, plus, doing necessary changes and depending on this module should be relatively easy

[millerjp]

Issue created and assigned to @HadesArchitect #634

[mhmdkrmabd]

@HadesArchitect Worth to be mentioned here that there's a planned refactoring process after finishing important tickets, we do regular refactoring process after couple of merged PRs, feel free to create a branch and don't worry about how tidy or clear the code is, we'll clean and improve the source code afterwards 👍.

[HadesArchitect]

It was literally my next proposal 😁 Seeing exceptions used as goto statement makes me cry 😅 вт, 17 дек. 2024 г., 18:47 Mohammed Abed El Hay Abu Baker < ***@***.***>:

…

@HadesArchitect <https://github.com/HadesArchitect> Worth to be mentioned here that there's a planned refactoring process after finishing important tickets, we do regular refactoring process after couple of merged PRs, feel free to create a branch and don't worry about how tidy or clear the code is, we'll clean and improve the source code afterwards 👍. — Reply to this email directly, view it on GitHub <#632 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANJLXJK4JBBB52YQN2RIHL2GBPRZAVCNFSM6AAAAABTW4EKCGVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTCNJZGY2DSNA> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

[mhmdkrmabd]

@HadesArchitect Not really, we're using try-catch nearly everywhere, and it's a good approach to use exceptions here instead of big if and if-else blocks, and, if we want to use if and if-if else-else blocks, nearly all of them will be wrapped in a try-catch block, so we're trying to shorten things, it might be confusing at first to realize that the condition which throws the exception and jump over the try-catch is the negative one, the project is huge already, and I'll do a refactoring process for it shortly, and we're very open for suggestions overall, and for a possible new style-guide, we can always discuss ideas