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

Proposal: Unified logging #14

Closed
daserge opened this Issue Aug 31, 2015 · 5 comments

Comments

Projects
None yet
5 participants
@daserge
Contributor

daserge commented Aug 31, 2015

Cordova unified logging

Note: The issue is associated with Cordova-lib refactoring proposal .

Existing architecture issues and proposed solutions

1. cordova-lib and cordova platforms have logging set up inconsistently

Current implementation details:

  • cordova-lib uses EventEmitter, which is further listened in cordova-cli and redirected to console calls,
  • Cordova platforms use console methods directly.

This leads to the following issue: log level of cordova-cli doesn't affect platforms at all.

Proposed solutions:
Variant A: Platforms can use EventEmitter passed as a dependency module via PlatformApi' interface (f.e. as a ctor parameter).
In case of CLI-mode cordova-lib EventEmitter will be used (and logs handling will be unified therefore).
In case of platform-standalone mode EventEmitter will be defaulted in the platform.
Variant B: Pass logger class from cordova-lib down to platforms and call its methods further.

Variant A is recommended as it separates aspects of events emitting from logging and is more flexible in terms of substituting logger module.


2. The log messages aren't standardized

Proposed solution:

  • logger module verbosity-based prefixing can be used (WARN :, ERROR:, etc.),
  • Emit objects rather than just messages in case of errors (f.e. use CordovaError extended with an error code).

3. Platforms logging mechanism does not allow plugging in a custom handling code/module (integration & custom transport levels)

Proposed solution:

  • Top-level cordova.on event can be used to subscribe for error events since cordova-lib' EventEmitter will be used,
  • Additionally logger module used in CLI or platform-standalone mode can be substituted to a pluggable one, for example winston,
  • Some multi-transport logger module can be used to use transports other than console (file, couchdb, etc.).

4. Errors from third-party components (ant, gradle, etc.) can't be easily distinguished

Proposed solution:
Attach to process.spawn stdout and stderr, supply caught errors with their source in meta object (f.e. extended CordovaError - see 2.).

@stevengill

This comment has been minimized.

Show comment
Hide comment
@stevengill

stevengill Sep 1, 2015

Contributor

Great writeup! I've been wanting to look at our logging support and standardizing it across our projects.

I like variant A and the idea that can try out different loggers.

I also like the idea of having error codes we define/standardize and use for cordova that persist among our various projects.

Contributor

stevengill commented Sep 1, 2015

Great writeup! I've been wanting to look at our logging support and standardizing it across our projects.

I like variant A and the idea that can try out different loggers.

I also like the idea of having error codes we define/standardize and use for cordova that persist among our various projects.

@muratsu

This comment has been minimized.

Show comment
Hide comment
@muratsu

muratsu Sep 1, 2015

👍
I'm all in for a standardized logging. Error verbosity will also give us the ability to debug things much more efficiently. This will make verbose mode much more useful.

muratsu commented Sep 1, 2015

👍
I'm all in for a standardized logging. Error verbosity will also give us the ability to debug things much more efficiently. This will make verbose mode much more useful.

@nikhilkh

This comment has been minimized.

Show comment
Hide comment
@nikhilkh

nikhilkh Sep 1, 2015

Contributor

This is great! I like variant A.

Contributor

nikhilkh commented Sep 1, 2015

This is great! I like variant A.

@purplecabbage

This comment has been minimized.

Show comment
Hide comment
@purplecabbage

purplecabbage Sep 1, 2015

Contributor

+1 Variant A

Contributor

purplecabbage commented Sep 1, 2015

+1 Variant A

@nikhilkh

This comment has been minimized.

Show comment
Hide comment
@nikhilkh

nikhilkh Jan 7, 2016

Contributor

Looks like most of this is implemented now. We should close this issue.

Contributor

nikhilkh commented Jan 7, 2016

Looks like most of this is implemented now. We should close this issue.

@daserge daserge closed this Jan 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment