feat: add Winston logging

eco-stake / restake

Auto-compounder script for Cosmos Validators using REStake

https://restake.app

MIT License

359 stars 303 forks source link

feat: add Winston logging #780

Closed freak12techno closed 1 month ago

freak12techno commented 2 months ago

Fixes #779.

adds Winston as a replacement to timeStamp() function
replaces all timeStamp() occurrences with Winston calls
Winston is set to pretty-print logs, assuming it doesn't have nested fields (can be configured to write in JSON, guess that's better to do in a later PR)
most of the data is now logged in a structured way (still a lot can be improved; better to verify it manually and see how to make it better)

here's how it looks like when I build and start it:

I already see some logs that do not make sense here (like this healthcheck start emoji and ...batch (as it's unclear what this batch is referring to), but not sure how to better update it.

Any suggestions on how to make it even better are welcome.

@tombeynon mind taking a look?

freak12techno commented 2 months ago

Also, some off-top thing: with .mjs it's somewhat difficult sometimes to figure out what type a specific variable is or what signature does this specific function has. This could be solved by moving to TS and adding strict typing, but this goes waaaaay out of scope of this issue lol. I suggest to plan moving to TS at some point, so that if you pass incorrect data to the function (I faced a few cases like this when I did return logger.info() instead of logger.info(); return, leading to incorrect type being returned and the app crashing due to that) won't even compile (and ideally won't ever pass the CI check and won't be merged into main, but that's another topic), do you think it's reasonable?

@tombeynon

freak12techno commented 1 month ago

hey @tombeynon did you have a chance to look into it?

tombeynon commented 1 month ago

@freak12techno so sorry for the delay. This is epic, thanks so much! I'm going to test it on my install for a day then we'll get it released.

On your other comments - totally agree, both about .mjs and Typescript. In all honesty this needs a complete re-write, I could explain the decisions/history that led to the current implementation but it's all excuses 😂 It's not a particularly complicated script especially if we abstract the signing logic, there's just a lot of technical debt in this version from the initial build.

In the meantime these changes are perfect and we'll use this logging implementation in the rewrite whenever it happens.

tombeynon commented 1 month ago

This is working great @freak12techno - I'm going to merge and release now. This is an awesome base for the new logging implementation, once it's merged we can take a look at some of the logs and see if they can be improved and are tagged correctly.

Thanks so much for all your work on this. Let me know if there's anything else you want to tackle, or raise an issues you want me to look at.

freak12techno commented 1 month ago

I'm all in for rewriting it in TS, but that'd require more work lol. (Seems like it'll totally benefit everybody though) I can probably also work on it a bit later.

tombeynon commented 1 month ago

Is there anything I can do in the meantime to make that easier? E.g updating the node packages, maybe changing those .mjs files?

freak12techno commented 1 month ago

I think no. Can you create an issue on that and assign me to it?