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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

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