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

Improve Logging #9545

Open
tsmaeder opened this issue Jun 2, 2021 · 3 comments
Open

Improve Logging #9545

tsmaeder opened this issue Jun 2, 2021 · 3 comments
Labels
epic epic issues consisting of multiple smaller issues logging issues related to logging

Comments

@tsmaeder
Copy link
Contributor

tsmaeder commented Jun 2, 2021

Recently I've been unhappy with the state of logging in Theia. In particular, since the ILogger interface has been "deprecated" in favor of overriding the console output streams, we can't turn logging on and off for a particular topic anymore. For example, I might want to debug the ProvidedTaskConfigurations class, but leave the rest of the system logging at info level. As I remember, an impetus behind this was that we wanted to control logging from libraries, as well. We'll need to think about this use case.
Secondly, we have no explicit policy what logging code should do and at what level. For example, we might require (as a coding convention) that every method entry/exit is logged at the trace level. (This is just an example, so don't start arguing yet ;-)). So I'm proposing the following plan of attack to improve logging:

  1. Improve logging infrastructure
  2. Define a logging policy and write it down: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#logging
  3. Add proper logging to at least some code to serve as an example
  4. Require proper logging (as defined in step 2) when reviewing PR's
@tsmaeder tsmaeder added epic epic issues consisting of multiple smaller issues logging issues related to logging labels Jun 2, 2021
@paul-marechal
Copy link
Member

we might require (as a coding convention) that every method entry/exit is logged at the trace level.

I'd argue that we should look into automatic instrumentation for this kind of use case (babel?).

(This is just an example, so don't start arguing yet ;-))

Oops.

As I remember, an impetus behind this was that we wanted to control logging from libraries, as well.

Avoiding the trouble of identifying third-party libraries individually, maybe we can put them under a common logging category like third-party? We currently monkey-patch the global console.log to use our loggers instead, not sure what to think about that. Maybe just make it simplier? Both to setup and maybe disable?

I might want to debug the ProvidedTaskConfigurations class, but leave the rest of the system logging at info level

If you find a nice way of implementing loggers for that kind of use case I'd be curious to review it. Feel free to ping me if you ever try things and want feedback on it.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jun 3, 2021

If you find a nice way of implementing loggers for that kind of use case I'd be curious to review it.

Well, all the Java logging frameworks do it. It can be as simple as

constructor(@inject(ILogger) logger) {
  this.logger= logger.child('MyClass');
}

What's the problem with this approach? Of course, that does not work for libraries and it is quite hard to implement via reflection, since Javascript doesn't allow for access to the call stack in a portable manner. But we could do it for our own classes.

@paul-marechal
Copy link
Member

paul-marechal commented Jun 3, 2021

Having one such logger per class sounds fine to me.

Only nitpick is that I think we can get inversify to do a bit of that work for us. I tried playing around with it, see gist PoC.

edit: line 27 to 52 are what usage of those inversify loggers looks like and line 89~99 illustrate different binding use cases.

The idea is to get the info about the logger's parent component from the inversify's context when resolving loggers. The logging API should be flexible enough to be able to configure things by hand, but if inversify has the info then let's leverage that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic epic issues consisting of multiple smaller issues logging issues related to logging
Projects
None yet
Development

No branches or pull requests

2 participants