Generalize logger usage between src and binary folders

[scorbajio]

This change looks into creating a general logger interface and implementing two different versions of loggers, a console and winston one, and it removes the winston imports from the src directory and brings them to bin. Also see #3922.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.05%. Comparing base (1147033) to head (015d620).

Additional details and impacted files

Flag	Coverage Δ
block	83.29% <ø> (ø)
blockchain	89.33% <ø> (ø)
client	?
common	98.49% <ø> (ø)
devp2p	86.73% <ø> (ø)
evm	72.98% <ø> (ø)
genesis	99.98% <ø> (ø)
mpt	90.07% <ø> (+0.30%)	⬆️
rlp	91.43% <ø> (ø)
statemanager	69.16% <ø> (ø)
tx	90.58% <ø> (ø)
util	81.96% <ø> (ø)
vm	57.20% <ø> (ø)
wallet	88.55% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[holgerd77]

Some loose thoughts while hovering through the changes here and also through the code base from the existing implementation (didn't have a closer look if you are already moving in some directions mentioned here, so these are just my thoughts, either as a confirmation or an implementation idea).

1. I think it makes sense that we put the console logger interface implementation into src (and keeping the winston thing out), since it does make sense to have one very simple logger "shipped" with the client-lib package, and the console logger should be unproblematic and not draw in dependencies or so, so nothing lost here, winston interface implementation should stay out though
2. Generally we should aim to have the code "out" (so: in bin) as minimal as possible, since this will be the code which will need to be replicated in a somewhat redundant way in the future (for client-stateless bin folder, client-portal bin folder, whatever). In this context we should have a closer look at the logging.ts methods. We should keep everything which is somewhat generic (and has no Winston dependency or very much Winston-specific code) inside lib (so that the generic functionality can be reused). I have no clear demarcation lines myself here, this will likely need a closer look at each method. If there is some functionality which only makes sense for a certain logger, just looking at the color part from function logFormat(colors = false) we might also still want to keep this in (lib), and then this is just not used (so: the color functionality) by the console logger
3. So, and here is where things get interesting 🙂 : so these logger utility methods will become in some way part of a (yet to be created) Public API of the client. These things should be documented mid-term at some point, also remain relatively stable, so that people can rely on these calls without something break along releases. I do not have the full picture myself here yet, but we can also evolve on this over time. Nevertheless likely already good to give this a start and move in the direction. I could e.g. imagine a set of 5-8 (partly static) clearly defined objects/classes at the end (of our development process) which we expose to the user, with the EthereumClient (and its associated API) being at its core, and with some side objects, helper classes like LogHelper, which people can then call into. Action Item: so, for the logger we can also start to "think in API terms" and a) maybe bundle relevant functionality in a static class LogHelpers inside the logging.ts file (I do not think that tree shaking plays any significant role here so high in the stack and for such few code, and this would make things easier to expose, and then also expose this LogHelpers class directly through the main src/index.ts file (so, that's the "goal" respectively the "end game" 🙂 ): everything which can be used "from the outside" should be well-defined and exposed in a controlled way here (and then also get some docs at some point).
4. Some additional notes for the things you mentioned in the chat:

• isInfoEnabled: not sure if this one specific use case in tx pool justify to integrate / maybe we look at the usage instead to see if we want to keep this one-spot-usage
• configure: also this one-spot thing, a bit more important usage in RPC though: I think I have some sort of generic solution to this and the above: I think we should provide the interface with an enum-like (see Gabriels work 🙂 ) dict with logger names like WINSTON, CONSOLE (or similar) and OTHER, and this needs to be set in the implementation so that name is set to WINSTON: and then for these one-spot instances we can do stuff like if (logger.name === 'WINSTON') { // do a direct call to the logger itself (so: the concrete Winston instance, maybe exposed by an anytypedlogger property in the interface, call also "shielded" by a check-bypassing call } else { error with 'call only supported for Winston logger' }. A bit hacky, but I would think justifiable for these one-time-spots (if there is a cleaner solution go for it for sure 😋 )
  • getLevel: do not find such a call in the code base on first search, not sure

5. Ok, this is an important one: Maybe it is/was a brain-dead idea from me in the first place to come up with something like a self-written console logger in the first place! Just realize that this (writing a logger) is something with a solid base complexity. So, maybe we go a bit back to the drawing board, and see what we actually want to achieve, which is a) prevent users having the burden of the huge Winston dependency set and b) have a logger which is also friendly to "library mode", so it can be turned on/off, respectively here https://logtape.org/manual/library is also context for using a logger within a library. So maybe it's rather the more practical solution to take existing simple logger code (just googled but did not find anything directly) and copy over or otherwise take a zero-dependency logger solution and do not go for a Winston <-> Console swap but for a Winston <-> Zero-Dep-Solution swap, I stumbled upon https://github.com/dahlia/logtape which might already be adequate, but I guess a second round googling would be useful
6. Going from 5., maybe its even adquate (leading to our goals) to skip the logging generalization alltogether and only swap the logger, e.g. with the one from 5.. I am not sure though, if especially this one fullfills also the visual needs (and other) we currently have. This would need some investigation.

I guess Action Item, especially from 5. and 6., might be: maybe take a one-day-break and rather go a bit deeper into the logger space as a whole, see what's there, see what others are suggesting and the like. 🙂 Then it would be great if you can provide the team with some overview what's there and what might be appropriate to choose!

Wow. A lot to think here. 🙂 This was useful for myself as well though. Need to especially think more about this "library mode" and an associated API and how to structure.

[holgerd77]

I am not getting what is happening here, think this can be removed (binding this to this, generally, binding should be avoided)?

[scorbajio]

There was somewhere in the client source code where the logger's info or debug function was be assigned to a new variable and used through it, which was causing issues with the new class implementation because this.logger was undefined when the function is used like that. Will double check this.