The Wayback Machine - https://web.archive.org/web/20201012021428/https://github.com/hyperledger/aries-cloudagent-python/issues/438
Skip to content

/ aries-cloudagent-python

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

Jump to bottom

Rationalize the logging in ACA-Py #438

Open
swcurran opened this issue Mar 26, 2020 · 6 comments
Open

Rationalize the logging in ACA-Py #438

swcurran opened this issue Mar 26, 2020 · 6 comments
Labels
good first issue

Comments

@swcurran
Copy link
Contributor

@swcurran swcurran commented Mar 26, 2020

While the team building ACA-Py has been pretty good at maintaining the logging, there is some inconsistency in the handling of logging. This task is to formalize the logging approach across the board, and then to do a pass through the code ensuring that the logging statements match the intention of the approach.

The first part (formalizing) is likely just creating something like a "logging.md" that outlines how contributors should be doing logging. Even better would be to find and reference (likely in "contributors.md") a well-known Python standard logging approach. Whatever is selected should be reviewed and agreed to by the team.

The second part includes ensuring the logging library is configured appropriately, any non-logging print statements are either removed or converted to logging calls, and that all logging is happening at the appropriate level (e.g. debug, warn, error, etc.).

Wrapping up the project will include adding guidance (or CI checks?) for existing and future devs on how they can make sure that the logging remains top notch.
@nacerix
Copy link

@nacerix nacerix commented Apr 2, 2020

I am interested in working on this issue.
@swcurran swcurran removed the help wanted label Apr 2, 2020
@swcurran
Copy link
Contributor Author

@swcurran swcurran commented Apr 2, 2020

Assigned to you @nacerix -- thanks. Please let us know if you have any questions.

Please look at the contributor guidelines and especially make note of the need to DCO sign off every commit. That is a requirement with all Hyperledger repos. Much worse if you find out about that later. :-)
@nacerix
Copy link

@nacerix nacerix commented Apr 3, 2020

sure, I will do
@nacerix
Copy link

@nacerix nacerix commented Apr 8, 2020 •
edited

Some proposals to start the discussion:

  1. Print is not a good idea
  2. Use Python standard logging module to:
  • You can control message level and filter out not important ones
  • You can decide where and how to output later
  • Write logging records everywhere with a proper level
  1. Write logging record everywhere with appropriate level and configure them later.
  • What is the appropriate level to use? (to do Add recommendations?)
  1. Recommend cascaded logger naming: use name as the logger name
  2. Capture exceptions with the traceback
  3. Use global logger instead of module level logger.
  4. Use JSON or YAML logging configuration over code
  5. If logging to a file, prefer rotating file handler
  6. Setting level names to avoid typos
  7. Performance: Wrap Expensive Calls in a Level Check
  8. Performance: Avoid Logging in code that is critical for performance
@swcurran
Copy link
Contributor Author

@swcurran swcurran commented Apr 8, 2020

Definitely agree on 1 :-) - that's one we want to be sure to eliminate.

I think the rest of your points are correct but leave it to the devs to both confirm and add guidance as to what is in place now and what we want to have.

@andrewwhitehead @nrempel -- can the two of you add your $0.02CDN to help kickoff this task?
@nrempel
Copy link
Contributor

@nrempel nrempel commented Apr 15, 2020

Thanks for picking this up @nacerix!

I agree with all of your points. We use the python logging module and normally make it available like this: https://github.com/hyperledger/aries-cloudagent-python/blob/master/aries_cloudagent/protocols/present_proof/v1_0/manager.py#L42

We allow setting of log level with a command-line parameter when you start the process but it is also possible to ingest a python logging config .ini file for more complex configuration: https://github.com/hyperledger/aries-cloudagent-python/blob/master/aries_cloudagent/config/logging.py

We use print() in a couple places early in the lifecycle of the process. If you can find a clean way to move that to the python logging module, go for it. I think we currently use print() any time before our logging config has been loaded.

Let me know if you have any questions!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue
3 participants
@nrempel @nacerix @swcurran
You can’t perform that action at this time.