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

💡 Wishlist item: use loguru as default logger #3509

Open
ff137 opened this issue Feb 10, 2025 · 5 comments
Open

💡 Wishlist item: use loguru as default logger #3509

ff137 opened this issue Feb 10, 2025 · 5 comments

Comments

@ff137
Copy link
Contributor

ff137 commented Feb 10, 2025

I have a lot of ideas for things that will improve ACA-Py -- and not from a "new feature" point of view, but from an "achieving stability" and "making things maintainable" point of view.

At the top of my list is the most mundane and boring of them all: improved logging.

In any project as complex as this one, logging is exceptionally important. Not for when things are working and going great, but for when things aren't working, and you want to quickly figure out why.

Therefore, I recommend Loguru

Loguru is a library that makes logging simpler, and is widely adopted (used by 108k repos).

My primary motivation for recommending this: it gives us trace logging, and bounded contexts.

Reasons for trace:

trace is an additional log level below debug, for when it's absolutely necessary to see the nitty gritty of every step. You should almost never actually have to use trace level, especially in production, unless things really hit the fan and you have no idea why something is erroring.

It helps to keep debug level less noisy, since debug is a more reasonable level to run in production. Debug logs should be informative without tracking every rudimentary operation. Those are moved to trace.

Trace also has the benefit of serving as a comment for developers to read what is going on. It doesn't need to get printed to stdout -- when someone reviews the code, the trace log lines can help to express in natural language what is going on.

Trace can also reveal inefficiencies, as it can reveal when a rudimentary operation is called when you don't expect it. (This seems to be a very real problem, but we don't want to clutter debug logs to find all those cases.)

Reasons for bounded contexts:

ACA-Py is an async web framework, and keeping track of which log is associated with which request can be exceedingly complex when multiple requests are served concurrently.

You wouldn't want to manually edit and append variables to every existing log call, in order to keep track.

So, loguru simplifies things by letting you bind variables to a context in a very simple way, e.g.:

bound_logger = logger.bind(body={"cred_ex_id": cred_ex_id, ...})
bound_logger.info("I don't need to mention the cred ex id, it'll get printed right here, automatically ->")

The way that bounded contexts are printed is highly customisable.

Other reasons:

Additionally, loguru has built-in features that have been implemented manually in ACA-Py -- e.g. file logging with rotation.

Other optional features that come with loguru:

  • lazy logging, colorized logging, customisable serialized logging, and more.

We don't need to implement any of these extras, but it's there.

Lastly, with loguru I'd be more than willing to give a comb-over to existing logging, and improve it all over the place. Without loguru, I think it's much harder to make truly meaningful improvements.

AI can streamline a lot of the mundane work required in improved logging -- with intelligent prompts and review, we can give a massive make-over to the logs in (relatively) no time.

First things first, how do we switch to loguru?

Refactoring to replace the existing logging should be simple enough:

  • implement a central log_config file, which sets up your desired config, and return this with a getLogger method.
    • here's an example of how we did it in our project (we called it get_logger, snake_case being more pythonic)
  • replace all import logging lines with import log_config as logging. And tada, we're using loguru instead.

Next, we can start using .trace and .bind -- maintainability goes through the roof.

Insert shia_labeouf_just_do_it.gif


If there's one additional item I can add to this wishlist item, it is: obfuscate sensitive data from logs.

Things like credential attributes can contain personally identifiable information, and we would want a simple way to switch that off from the logs, and never record these at any log level. Using loguru can make this simpler. But, this should be an issue on its own.


There you have it. I understand that changing core functionality like logging feels like a "big change", so I welcome any and all feedback, especially any concerns or extra considerations.

One consideration I can think of is that it ought to be a non-breaking change, and existing ACAPY_LOG_CONFIG specifications (custom formatters and handlers) should continue to work in exactly the same way. This is where most of the attention for this will go.

Your comment, input, or thumbs up/down will be much appreciated.

@swcurran
Copy link
Contributor

Sounds good to me — a significant improvement. How do we get this started?

@swcurran
Copy link
Contributor

Added to the Agenda for the Maintainers meeting — 2025-02-11

@ff137
Copy link
Contributor Author

ff137 commented Feb 11, 2025

Sounds good to me — a significant improvement. How do we get this started?

I can test things locally, using a similar loguru config file as we have here.

Then I'll look into how formatters and handlers can currently be customised, and accommodate for that.

Will probably take some time before I can focus on it. I don't see it being too complicated, but one never knows until one gets started. So, any thoughts or concerns are welcome, before I give it a shot

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Feb 13, 2025

This should be used in conjunction for declarative configuration; loguru-config

@esune
Copy link
Member

esune commented Feb 13, 2025

I think this is a great idea and I'll be happy to help/support @ff137 as needed with this - I was just talking about logging with @loneil and how it would benefit from a revamping this morning.

A few thoughts on the topic:

  • Changing imports: I believe we had been reluctant to do such a broad codebase rename/replace in the past, however I feel like this is inevitable at this point in order to be able to leverage advanced/beter logging features. And as @ff137 stated, we can take this as an opportunity to clean-up existing logging logic and handling (we have scenarios where errors are printed to console when they should be warnings, and others).
  • Obfuscating sensitive data: this would be very welcome, it is a feature that is definitely missing currently.
  • As @WadeBarnes suggested, I would enable support for configuration files from the get go using the linked library. One of the main use-cases for this is the possibility of configuring serialized logging (JSON) for deployments running with log aggregation backends such as Kibana (what we use in OpenShift). Not having JSON logging causes multi-line logs (e.g.: stack traces) to be ingested incorrectly and make troubleshooting even more difficult.
  • We do have a trace handler for many of the API requests supported by Aca-Py: maybe this is a good use of the trace logger mentioned.
  • We should ensure that functionality focused on qualifying log statements with the wallet/tenant id that is performing the operation continue to be supported, and possibly get even improved (i.e.: easier extensibility for plugins to add/remove context in logs) as this is an incredibly useful feature for multi-tenant scenarios.

The questions I have gravitate around the backwards-compatibility with current log configuration and log implementation in plugins: I think it will be inevitable to potentially introduce a breaking change - if we provide a migration guide it should be virtually painless - as maintaining support for both specifications seems like it would generate unnecessary complexity and overhead (assuming it is even possible).

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

No branches or pull requests

4 participants