stratisproject / StratisCore

MIT License
42 stars 49 forks source link

Add electron-log dependency for improved Core logging #338

Closed sondreb closed 4 years ago

sondreb commented 5 years ago
sondreb commented 5 years ago

Here is a screenshot that shows example from Windows, previously the console.log in main.ts ends up in shell, not in the browser console dialog. Now it additionall ends up in the log.log file. It is possible to further improve this, and push the logs from main.ts into the rendered thread, which will make them appear in the browser debug console as well.

image

Reference for electron-log library: https://github.com/megahertz/electron-log

sondreb commented 5 years ago

If there are any sensitive details in the arguments, this would be logged in this setup. Perhaps a generic "log-cleaner" could be built for both Core and FullNode, so that when someone forgets to annotate the methods in FullNode to not log, it still won't log secrets.

ianadavies commented 4 years ago

Hi there,

Thanks for submitting a PR, please note that we have a simple static logger which can be found in logger.service.ts and is mostly used throughout the rest of the app, but there are probably places where it is missed. So I guess if we went down this route it would need to implemented there also to keep consistency with the rest of the application.

The console logging is very useful during debugging so I guess it would make sense to log to both the file log and the console. I would think making this optional would make sense through a startup parameter as these logs can get big and we would then need to then manage that.

sondreb commented 4 years ago

The logger.service.ts is part of the Angular application, this is a logger for the main Node-thread running in Electron. It can be used to log important events that occurs in the main thread, such as launching and failed launches of the background daemon. The existing logger runs in the rendered thread and is not accessible from the main thread, except through IPC. If there is issues with the actual Angular-code/runtime/rendered thread, then no logs would be written. The logger.service.ts does not have local disk access either, so it must use IPC to send logs to main thread, which has disk access.

The existing logger in the logger.service.ts that runs on the renderer-thread, does not write any logs to disk. It is simply a wrapper for console.log. There are also some console.log entries in the main UI code that should be cleaned up, not too many.

This PR contains just a very few log entries and disk size/log size should not be an issue at all.

What should be considered, is adding some logic that forwards important messages from logger.service.ts and sends it to the main thread for storing on disk.

mrtpain commented 4 years ago

I agree with @sondreb I can take a deeper look at this PR and make implement needed changes to the logger.service.ts. Stratis Swap current uses electron log to write known and unknown caught errors to disk so when users are experiencing issues, they can be supported through sending/checking their FN logs and Core logs. The electron logs can be made to create a new file daily so users don't have to pick or look through a heavy log of multiple days of data. It just an extra step to supporting the users.