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

feat: add Winston logging #780

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

freak12techno
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@tombeynon
Copy link
Contributor

@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
Copy link
Contributor

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.

@tombeynon tombeynon merged commit 8c5719a into eco-stake:master Jun 3, 2024
@freak12techno
Copy link
Contributor Author

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.

@freak12techno freak12techno deleted the add-winston-logging branch June 3, 2024 10:38
@tombeynon
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring: use proper logging solution
2 participants