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

Introduce or implement logging framework for the SDK #926

Closed
2 tasks
kelsos opened this issue Jan 21, 2020 · 6 comments · Fixed by #1045
Closed
2 tasks

Introduce or implement logging framework for the SDK #926

kelsos opened this issue Jan 21, 2020 · 6 comments · Fixed by #1045
Assignees

Comments

@kelsos
Copy link
Contributor

kelsos commented Jan 21, 2020

Description

This is a proposal about improving and making the logging in the SDK more manageable.
It could be done by either using some popular framework (well maintained and tested) or by writing our own utility to properly handle logging in the SDK.

The major issue with our current approach comes from the fact that there are two different classes of logging:

  • Configurable: Redux-logger, matrix-logger
  • Non-configurable: console.log, console.error

Everything that is logged by the first class of loggers is easy to manage and turn off when logging is not needed.

For the second category, there is no easy way to currently turn them off and they always appear in the console. While this can be sometimes useful for debugging purposes the verbosity of the messages can be sometimes a bit overwhelming.

After deciding what direction we will follow in regards to logging it is important to decide on log levels for each console.log call in the SDK codebase. So messages like the signature messages Signing x Message can be at info log level for example.

Acceptance criteria

  • The number of messages varies depending on the log level
  • The user of the SDK can completely turn off logging if needed.
  • Implement our own logger class or use a dependency (look into logger dependencies)?
  • Nice and informative logging both on node.js and on the browser.

Tasks

  • Evaluate thirdparty dependencis vs first party implementation
  • Evaluate log level of various console.log entries
@christianbrb christianbrb added this to the Product Backlog milestone Jan 22, 2020
@kelsos
Copy link
Contributor Author

kelsos commented Jan 27, 2020

Looking through the matrix javascript sdk they seem to use https://github.com/pimterry/loglevel

@nephix
Copy link
Contributor

nephix commented Feb 5, 2020

At the same time we should be able to disable logging when running unit and e2e tests in raiden-ts. Currently, the logging output is more than overwhelming and it's very difficult to spot errors and exceptions.
One possible solution might be to turn off logging, if process.env.NODE_ENV === 'testing'

@kelsos
Copy link
Contributor Author

kelsos commented Feb 5, 2020

Yes I would also argue for disabling logging completely on the CI. The amount of output is certainly overwhelming.

@andrevmatos
Copy link
Contributor

The CI logging has proven invaluable for me several times on test failures. I'd argue the perfect approach would be to somehow get jest/CI to keep logs, but show them only for failed tests instead of everything.

@kelsos
Copy link
Contributor Author

kelsos commented Feb 5, 2020

There is this issue for that jestjs/jest#4156 but it seems to have been closed.

@taleldayekh taleldayekh added the 3 label Feb 6, 2020
@kelsos kelsos removed this from the Product Backlog milestone Feb 6, 2020
@andrevmatos
Copy link
Contributor

loglevel seems lovely! Has native typing, is small, has no dependency and is extensible through plugins. I'd recommend we avoid the global logger object and use specific ones for each instance, possibly keyed like raiden:<address>, to allow specifying different logger per instance (and possibly having the name printed on log lines for debugging). This can be done by having log as a RaidenEpicDeps, exposed using a private raiden.log property in Raiden itself, as well as used in epics and utilities. The later can be a little bit troublesome by having to pass the logger as a parameter to utility functions (or maybe whole deps where relevant), but I'd say it's worth it.

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

Successfully merging a pull request may close this issue.

5 participants