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

Debug Logging #167

Merged
merged 7 commits into from
Jun 9, 2017
Merged

Debug Logging #167

merged 7 commits into from
Jun 9, 2017

Conversation

hexa00
Copy link

@hexa00 hexa00 commented May 31, 2017

This adds debug log support, see the patches commit log for more info

The only thing missing is to be able to call logging outside of an injectable context.

Like to be able to replace the ConsoleLog used in connection.ts for example, I don't know how to manage that if anyone has ideas ?

@@ -39,6 +40,8 @@
"devDependencies": {
"@types/chai": "^3.5.1",
"@types/webdriverio": "^4.7.0",
"@types/yargs": "^6.6.0",
"bunyan": "^1.8.10",
Copy link
Contributor

@kittaakos kittaakos May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit, I did not look into the complete change yet, but I am positive, that the logging framework is not a dev dependency but a runtime one. In a nutshell; it should go the dependencies. Please correct me if I am wrong.

Copy link
Author

@hexa00 hexa00 May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this one is to be able to run:
npm start::backend as node ./lib/backend/main.js | bunyan
So it enables piping the output to the buyan executable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the npm run start* command are seen as dev or prod ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hexa00

  • if something is required at runtime it should go to dependencies
  • if something is required by npm scripts then it should go to devDependencies
  • if something is required at runtime and by npm scripts then go to dependencies as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you already have bunyan as dependencies in theia-core, so keeping it here as dependencies should be OK

Copy link
Author

@hexa00 hexa00 Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that clarifies things so I was right to have it in devDeps in that commit

@@ -33,6 +34,8 @@
"devDependencies": {
"@types/chai": "^3.5.1",
"@types/webdriverio": "^4.7.0",
"@types/yargs": "^6.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yargs goes to dependencies then its type definitions should go there too. Rule; put them into the same place.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that was only true for theia-core since it was consumed by the examples...
Why is it true for the examples?

@@ -23,6 +24,7 @@ export interface BackendApplicationContribution {
export class BackendApplication {

private app: express.Application;
@inject(ILogger) private logger: ILogger;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, we just start to mix ctor and field injections. I am fine with that, I just do not know if your change was intentional or not.

Copy link
Author

@hexa00 hexa00 May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I just wanted such a helper to be out of the "core" depencies, I though a property was a good way to do that..
Other opinions on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inject it into the ctor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean in general, using ctor or property injection... I can put it into to ctor it just seems like it will become bloated at some point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks, I'll change it to ctor

alias: {
'dtrace-provider': path.resolve(__dirname, './webpack_empty.js'),
fs: path.resolve(__dirname, './webpack_empty.js'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm could you elaborate ?

I did not do much research on webpack yet... I just followed the bunyan doc, So I'm not sure about the interactions between webpack.config.web fs:'empty' and the alias I have here...

Copy link
Contributor

@kittaakos kittaakos May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs has to be mocked for the browser and it is already mocked. I would expect that if you remove the above-referenced line, it will still work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK Seems like mocking wiht a simple : 'empty' is way nicer too I'll check if that works

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'empty' doesn't work there... but indeed I can remove fs

fatal(obj: Object, format?: any, ...params: any[]): Promise<void>;
}

export { ILogger, Logger }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought bunyan will not be visible for the client code.

Copy link
Author

@hexa00 hexa00 May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it needs to be visible to be able to create the bunyan logger in the backend, cache the proper levels in the frontend and use the bunyan types where needed...
I could have reimported bunyan in all the places that I needed it but it seemed redundant..
Another optino would be to export it as BunyanLogger maybe ?

But oviously we should not use Logger directly to log.

const rpcProxy = connection.createProxy<ILogger>("/logger");
let cachedLevel: number | undefined = undefined;

const levelFromName: any = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need an explicit any type here, I prefer to use specific types or let ts compiler infer them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was giving an error calling levelFromName[p] , since there was no index type, actually I'm not sure how to specify that... I need to check it out.. or if you have a idea?

* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/

import * as Logger from 'bunyan';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be used only in node module and Log interfaces should not have dependencies on bunyan.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right

* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/
import { ContainerModule } from 'inversify';
import { ILogger, Logger } from '../common/logger';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger should be imported explicitly from bunyan here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

const ILogger = Symbol("ILogger");

interface ILogger {
addStream(stream: Logger.Stream): Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have to expose all available capabilities from bunyan?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that was my first I idea I thought let's just expose bunyan. Thus my proxy approch etc..
But looking at your review I now think this was not the right approach especially since we need to control what we send as objects over JSON-RPC..

But I think we would like at least to:

  • child : So that extensions can log in their scope
  • level: Get and Set so that we can change the level at runtime

The streams I don't think we can send that over json-rpc given that it's using a nodeJS stream object.
Same with the serializers they're a complex object.

So we could expose:

  • log level functions
  • child creation
  • logging functions
    And the logging functions would be string only over json-rpc to avoid sending random objects over json-rpc,
    But we could wrap around a format string interface to be used by the user.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind the child creation that won't work... we would have to reflect it in the interface... and manage that...
So basically we could just expose the log level functions and the logging functions for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have something simple done first and then look at other features and add them if we have a need for them.

  1. child sounds good
  2. if we provide setters for levels we also need the client interface to receive notifications about it:
export interface ILoggerClient {
  onLevelChanged(event: {
     loggerId: string; // some kind of and id to identify a responsible logger in case if we support children
     oldLogLevel: number;
     newLogLevel: number;
  }): void;
}

And ILogger can look like:

export interface ILogger {
    readonly id: string;
    error, info, warn ...
    createChild(id: string): ILogger;
    readonly onLevelChanged: Event<{oldLogLevel: number, newLogLevel: number}>
}
  1. logging functions: we can have params, for now it will be up to users to provide proper JSON objects. Later it would be nice to add serializers support generally to JSON-RPC proxies, so we can use any type, like URI in FileSystem interface. The same serializers mechanics can be used to customize serialization/parsing logging params. We should do it separately from this PR.
  2. there are attempts provide streaming over JSON-RPC: Add streaming support microsoft/language-server-protocol#182. We should look at it also separately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Create child creates a new Bunyan Logger instance, so we would need to create a new set of LoggerClient/LoggerServer on our end to manage that... Or find a way to map that...

  2. Why do these have to be notifications ?
    This is a singleton interface so setting the level on the Logger once the promise is resolved would be ok no ?

  3. OK I like that idea

  4. OK nice

Copy link
Member

@akosyakov akosyakov Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why cannot we extend ILoggerServer to support multiple loggers? like:
export interface ILoggerServer {
   log(id: string, level: LogLevel, messsage: string, params: any[]);
}
  1. true

Copy link
Member

@akosyakov akosyakov Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that params collapse with a cancellation token, you can just avoid using varargs in the server interface to overcome it, just use any[]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about the second point again, if you use a logger with the same id on the frontend and backend sides, and the backend changes the level how the frontend gets to know about it? it could be updated with notifications... or we just prohibit such usages

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We can it's just more logic, but OK I do think it's useful for extensions or components so let's do it.

Re token: Yes so params: any[] no ...

About 2, yes good point I do think it would be better to have a notification then, prohibiting the usage would complicate things

src: Promise<boolean>;
trace(): Promise<boolean>;
trace(error: Error, format?: any, ...params: any[]): Promise<Promise<void>>;
trace(buffer: Buffer, format?: any, ...params: any[]): Promise<Promise<void>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer is also from node

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed


const ILogger = Symbol("ILogger");

interface ILogger {
Copy link
Member

@akosyakov akosyakov Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the following design:

  • a minimal API for the logger server
export namespace LogLevel {
  export const ERROR = 2;
  export const WARN = 1;
  export const INFO = 0;
}

export interface ILoggerServer {
  logLevel(): Promise<number>;
  log(logLevel: number, message: string, ...params: any[]): void;
}
  • a logger service using the logger server that defines a convenient API and do level checking
export interface ILogger {
   error(message: string, params: ...any[]);
   warn(message: string, params: ...any[]);
   info(message: string, params: ...any[]);
}

export class Logger implements ILogger {

  protected readonly logLevel: Promise<number>;

  constructor(
    @inject(ILoggerServer) protected readonly server: ILoggerServer
  ) {
    this.logLevel = server.logLevel();  
  }

  protected log(logLevel: number, message: string, ...params: any[]): void {
    this.logLevel().then(level => {
      if (level <= logLevel) {
         this.server.log(logLevel, message, ...params)
      }
    });
  }

  error(message: string, ...params: any[]): void {
    this.log(LogLevel.ERROR, message, params);
  }

  info...
}
  • a bunyan implementation of ILoggerServer: BunyanLoggerServer

I think it has a clean separation and easier to grasp compared to the proxy logic in loggerServerModule below. Also, we should always remember that ILoggerServer is special, we cannot just send a random js object with the JSON-RPC, e.g. maybe we should remove ...params from the interface since it has any type. Other thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like:

  • The clean separation of interface/implementation
  • Proxy logic was fun but I did not know about the JSON-RPC limitations, so it doesn't make sense now, so going without it is fine.

I think we should go without params: any[] like you propose for the log server function, and resolved that before sending it over to the server.

I'll rework it as such and send a v2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep params, if it breaks it is still fixable by the user, we don't do anything with them, they just should make sure that passed params are JSON objects

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to validate that they are valid json objects at build time ? or even at runtime ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, it is very subtle, anything can be a valid JSON object if you know what you are doing, like if you are fine that functions are omitted

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, in C++ you have somethind like std::is_pod (Plain Old Data) for that... so I was wondering
I'll document it in the guide I plan to write on this

This patch adds debug logging support to the frontend and backend.

It adds a logging interface over JSON-RPC to a bunyan based server on the
backend.

This can be accessed by injecting a ILogger into a class and calling
bunyan like functions like so:

this.logger.debug('This is a debug message);
this.logger.info('This is an info message);

Note that since this interface is over JSON-RPC the log functions return a
Promise but since logs can be async, waiting for this promise to resolve
is not needed and it can be called as a normal function call.

Note also that to minimize the traffic over JSON-RPC and string
concatenation the log level is cached by the frontend so that the logs are
not sent over JSON-RPC if they do not meet the cached minimum level.

This cache is reset if functions that modify the minimum level are called.

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
@hexa00
Copy link
Author

hexa00 commented Jun 7, 2017

@akosyakov If you could please review this new version ? I think it addreses the comments.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, it looks really good, just some small remarks, see comments.

export const TRACE = 10;
}

export const ILoggerServer = Symbol("ILoggerServer");
Copy link
Member

@akosyakov akosyakov Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to move ILoggerServer, ILoggerClient and ILogLevelChangedEvent to another file, like logger-protocol.ts? To me, they are implementation details of ILogger a client does not need to be exposed directly to them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

setLogLevel(id: number, logLevel: number): Promise<void>;
getLogLevel(id: number): Promise<number>;
log(id: number, logLevel: number, message: string, params: any[]): Promise<void>;
child(obj: Object): Promise<number>;
Copy link
Member

@akosyakov akosyakov Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you trying to achieve by using Object type? Prohibit primitives? Then you should use object. Object is a common type for all types including primitives, it is inconvenient to use it on an implementation side. I think you want child(obj: any): Promise<number> or child<T extends object>(obj: T): Promise<number>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, prohibit primitives, I guess I was misslead by the bunyan d.ts with
child(obj: Object, simple?: boolean): Logger;

I reading about it I think I'll use the object type, but I don't see why I would need to define that it could be extended ?

Should be like child(obj: object): Promise<number>;
Since I intend to pass only { key: value} with key and value being primitives

constructor(
@inject(ILoggerServer) protected server: ILoggerServer,
@inject(LoggerWatcher) protected loggerWatcher: LoggerWatcher,
@unmanaged() childOptions?: Object) {
Copy link
Member

@akosyakov akosyakov Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use inversify factories instead. unmanaged is a hack to overcome inheritance issues.

* bunyan documentation for more information.
*/
constructor(
@inject(ILoggerServer) protected server: ILoggerServer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly

*/
constructor(
@inject(ILoggerServer) protected server: ILoggerServer,
@inject(LoggerWatcher) protected loggerWatcher: LoggerWatcher,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly

private _logLevel: Promise<number>[] = [];

/* Root logger has id 1. */
private rootLoggerId = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly, static?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly but I see no reason for static..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it is static. It does not belong to the instance but to the class. In other words, if you create three logger instances, why would it have its own property rootLoggerId?

Copy link
Author

@hexa00 hexa00 Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, I thought of it more in terms of visibility, static is public and that's a very internal private detail of the implementation

* bunyan documentation for more information.
*/
child(obj: Object): ILogger {
return new Logger(this.server, this.loggerWatcher, obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use an inversify factory here:

const LoggerFactory = Symbol('LoggerFactory')
type LoggerFactory = (options?: any) => ILogger;

const LoggerOptions = Symbol('LoggerOptions')

class Logger {

    constructor(
        @inject(ILoggerServer) protected readonly server: ILoggerServer,
        @inject(LoggerWatcher) protected readonly loggerWatcher: LoggerWatcher,
        @inject(LoggerFactory) protected readonly factory: LoggerFactory,
        @inject(LoggerOptions) @optional() options?: Object
    ) { }

    child(obj: Object): ILogger {
        return this.factory(obj);
    }

}

// in bindings
bind(LoggerFactory).toFactory(ctx =>
    (options?: any) => {
        const child = new Container({ defaultScope: 'Singleton' });
        child.parent = ctx.container;
        child.bind(LoggerOptions).toConstantValue(options);
        return child.get(ILogger);
    }
);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ho yes indeed!! Thanks for the code :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also need child.bind(ILogger).to(Logger).inTransientScope(), otherwise you always will get a singleton instance from the parent container

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, very nice this child container thing

@@ -24,7 +25,8 @@ export class BackendApplication {

private app: express.Application;

constructor( @inject(ContributionProvider) @named(BackendApplicationContribution) private contributionsProvider: ContributionProvider<BackendApplicationContribution>) {
constructor( @inject(ContributionProvider) @named(BackendApplicationContribution) private contributionsProvider: ContributionProvider<BackendApplicationContribution>,
@inject(ILogger) private logger: ILogger) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use protected where it is possible, I am not a big fun or private, subclasses should be able to access the state of superclasses

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also readonly when a property cannot be changed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will now default to protected readonly

export class Logger implements ILogger {

/* Log level for the loggers. */
private _logLevel: Promise<number>[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why it is an array, not just a promise?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep the different child loggers log levels... since they may differ from the root

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot each instance of logger keep it is own _logLevel based on id? What actually seems to happen in practice because you always do _logLevel[id], I just suggest using _logLevel directly instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I guess the code evolved like that and I did not realize it :)
So yes I'll switch that


/* Update the root logger log level if it changes in the backend. */
loggerWatcher.onLogLevelChanged(event => {
this._logLevel[this.rootLoggerId] = Promise.resolve(event.newLogLevel);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about child loggers? Should not they also be updated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No child loggers are instance based, so if you create a child Logger in the frontend, it's frontend controlled only, same with backend.
So you can't access a frontend created child logger from the backend and change it's level...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not wathicng then only be done when this.id === Logger.rootLoggerId and looks like this._logLevel = Promise.resolve(event.newLogLevel)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this is implied right now given that's it's the only one that can update, but yes good thing to change

setLogLevel(logLevel: number): Promise<void> {
return new Promise<void>((resolve) => {
this.id.then(id => {
this._logLevel[id].then(oldLevel => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe? I think this._logLevel[id] can be undefined.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, I can add a check and a log

export interface ILogger {
child(obj: Object): ILogger;
setLogLevel(logLevel: number): Promise<void>
getLogLevel(): Promise<number>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it can be synchronous, you cache a log level anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind, you cache a promise, sorry

* @returns a Promise to the log level.
*/
getLogLevel(): Promise<number> {
return new Promise<number>(resolve => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not return this._logLevel[id]?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah part of it is just plain wrong I don't know how it got there like that... should be:

   getLogLevel(): Promise<number> {
        return new Promise<number>(resolve => {
            this.id.then(id => resolve(this._logLevel[id]));
        });
    }

I should guard about a non existent id too..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn not return directly since id is a promise...

@hexa00 hexa00 force-pushed the logging branch 2 times, most recently from 281aab6 to ffcef06 Compare June 8, 2017 13:28
@hexa00
Copy link
Author

hexa00 commented Jun 8, 2017

@akosyakov See this v3, the comments should have been addressed.
Thanks for the great review btw !!

@hexa00
Copy link
Author

hexa00 commented Jun 8, 2017

Oops I had forgotten a file in the 1st commit, fixed now

@inject(ILoggerServer) protected readonly server: ILoggerServer,
@inject(LoggerWatcher) protected readonly loggerWatcher: LoggerWatcher,
@inject(LoggerFactory) protected readonly factory: LoggerFactory,
@inject(LoggerOptions) @optional() options: object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a type should be object | undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes optional indeed

}

/* Fetch the log level so it's cached in the frontend. */
this.id.then(id => this._logLevel = server.getLogLevel(id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be this._logLevel = this.id.then(id => this.server.getLogLevel(id)), otherwise this._logLevel can be undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ho right good catch

Antoine Tremblay added 6 commits June 8, 2017 14:43
yargs will be used in a following patch more extensivly.

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
This patch adds a --loglevel option to the command line to specify the
debug log level on the browser and electron example.

See node main.js --help for usage.

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
This patch adds the npm run start:backend:debug to the browser example to
start the backend with a debug log level.

It also add the npm run start:debug command to the electron example for
the same purpose.

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>
@hexa00
Copy link
Author

hexa00 commented Jun 9, 2017

Fixed the last comments...
@akosyakov ok to merge ?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it actually works yes please :) BTW there can one find logs from the client and server after this PR?

@hexa00
Copy link
Author

hexa00 commented Jun 9, 2017

hahah yes it works :)
You will find the logs in the backend console starting with npm start:backend:debug and starting a terminal you will see:

[2017-06-08T18:47:39.296Z]  INFO: Theia/17039 on elxa4wqvvz1: Theia app listening on port 3000. [ [] ]
[2017-06-08T18:48:51.158Z] DEBUG: Theia/17039 on elxa4wqvvz1: Started terminal id: terminal-1

Event with colors! :)

Note a I added a terminal log mostly to show how it's done in the frontend I can remove it later

@hexa00 hexa00 merged commit bee0ded into eclipse-theia:master Jun 9, 2017
@hexa00 hexa00 deleted the logging branch June 9, 2017 11:00
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.

3 participants