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

Logging config #2

Merged
merged 3 commits into from Dec 13, 2021
Merged

Logging config #2

merged 3 commits into from Dec 13, 2021

Conversation

Goldziher
Copy link
Contributor

This PR adds a default logging config and a way of setting logging easily:

  1. I created a pydantic model for the logging.config.dictConfig expected parameter.
  2. I set the defaults in this model.
  3. To modify or extend the base config, the user is expected to subclass this pydantic model and pass it to the app when instantiating.
  4. If None is passed by the user to Starlite(), logging will be disabled.

@Goldziher Goldziher self-assigned this Dec 13, 2021
Copy link
Contributor

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

I really like the idea of emphasizing logging, and especially for something like a project-creation CLI. I think that's missing from a lot of microframeworks.

In terms of the implementation, I'm not 100% sure about whether:

  1. Coupling the logging setup to the app is necessary. Django does do this sort of the same way - they call dictConfig behind the scenes for you, but it would also be possible to leave it out of the app and just provide a setup_logging on_startup callable when creating a project. That wouldn't hide away the one line of code that gets users to understand how the setup works.

  2. If creating a model for the dict config is necessary. I get that there's an upside in terms of validating input (and protecting users from themselves), but in some ways it feels like we'd just be obscuring the already well-known python API.

I'll think about it some more. Do you have any thoughts re ^ @Goldziher?

@Goldziher
Copy link
Contributor Author

I really like the idea of emphasizing logging, and especially for something like a project-creation CLI. I think that's missing from a lot of microframeworks.

In terms of the implementation, I'm not 100% sure about whether:

  1. Coupling the logging setup to the app is necessary. Django does do this sort of the same way - they call dictConfig behind the scenes for you, but it would also be possible to leave it out of the app and just provide a setup_logging on_startup callable when creating a project. That wouldn't hide away the one line of code that gets users to understand how the setup works.
  2. If creating a model for the dict config is necessary. I get that there's an upside in terms of validating input (and protecting users from themselves), but in some ways it feels like we'd just be obscuring the already well-known python API.

I'll think about it some more. Do you have any thoughts re ^ @Goldziher?

Well, as to the points:

  1. yes its not required. Perhaps a callable is a better way of handling this - i am not sure. I thought that probably most more junior users will actually not setup logging by default - so I wanted to reduce the amount of moving parts. But maybe this is a mistake?
  2. I created a model to prevent users from making mistakes - it's not very thorough though, so users can still make mistakes. I personally find the python logging interface to be horrible and hard to figure out. If you have thoughts on how to improve - by all means.

@JonasKs
Copy link
Contributor

JonasKs commented Dec 14, 2021

Maybe consider shipping with loguru?

@sondrelg
Copy link
Contributor

I think one thing this project should strive for, is not deviating too far from Starlette or other starlette-derivatives. Being able to drop-in replace your project app and have things work with your existing configuration is a big plus.

Shipping with loguru would break my projects, and it's just one of several similar libraries (e.g., structlog) that do fancy logging. I think shipping with it would be locking us in with a third party lib without a great reason 🙂

@JonasKs
Copy link
Contributor

JonasKs commented Dec 14, 2021

I personally find the python logging interface to be horrible and hard to figure out.

Loguru is created to solve this problem, and nothing else. I personally don't use it in my projects, but in my opinion it does a great job of simplifying setup.

Being able to drop-in replace your project app and have things work with your existing configuration is a big plus.

Not sure if being able to drop-in replace should be something to strive for, it seems very limiting for an opinionated framework.

I think shipping with it would be locking us in with a third party lib without a great reason 🙂

While loguru is configured in a different way, it seems to have all the same options and possibilities. Not sure how that would break your projects?

I suggest adding loguru with pre-configured correlation IDs etc. The only thing a end user would have to do, is to add their extra handlers if they want external logging, which could easily be documented.

They can also live side by side, if desired:

import logging

from loguru import logger as logger_loguru
logger_normal = logging.getLogger(__name__)

logger_loguru.critical('hello world')
logger_normal.critical('hello world')

bilde

@JonasKs
Copy link
Contributor

JonasKs commented Dec 14, 2021

Updated my previous comment.

@sondrelg
Copy link
Contributor

What's the upside of using loguru vs structlog or vanilla logging with something like the rich logging handler?

@JonasKs
Copy link
Contributor

JonasKs commented Dec 14, 2021

That it is user friendly. You literally say poetry add loguru and then you do from loguru import logger; logger.info('hello world') and you have a fully colored, informative log message output. It don't require any setup.

As with this library, it is opinionated. It has defaults, handlers for file rotation etc. These defaults could be changed to include correlation-ids, API paths etc. That way it requires zero setup from the end user. If they want to configure any logs, they can either use the built in logging.getLogger() or loguru.logger.add().

Implementation on starlite logs could maybe even be something like this:

try:
    from loguru import logger
except:
    import logging
    logger = logging.getLogger(__name__)

I'm not saying it's the way to go, but I think it could be interesting to see its upsides and downsides.

@Goldziher
Copy link
Contributor Author

Interesting.

For me the main point of logging is to output useful, machine processable, information that can be searched. E.g. elastic search, data dig etc.

Colorized output really doesn't matter, although it might be cool.

The point really is simplicity. I wait the user to have working logging from the getgo, and still retain control of configuration.

Thoughts? I am not against Amy solution suggested by you guys so far.

@sondrelg
Copy link
Contributor

It might make sense to build something like this into a starter-project template, but I think the framework should be very careful about constraining the user in any way - because you cannot predict user requirements.

Maybe not the best example, but as an illustration: let's say a user needs a dependency A, and A has a downstream dependency B that is shared by loguru. What happens if loguru pins the requirement to a range that is incompatible with A? The answer is, they're now blocked from upgrading Starlite, right. The python built-in logging doesn't have this problem.

@JonasKs
Copy link
Contributor

JonasKs commented Dec 14, 2021

It might make sense to build something like this into a starter-project template

Yeah, this might be true for all logging and a bunch of extra features honestly. Logging can be hard, but a CLI would probably be sufficient.

Maybe not the best example, but as an illustration: ...snip

While loguru don't have any requirements, it's a valid concern. Seems like the tiangolo-world already struggles a bit with this.

(I managed to delete the branch when trying to click comment, woopsie. GitHub saved me with a restore button)

@JonasKs JonasKs deleted the logging-config branch December 14, 2021 20:55
@JonasKs JonasKs restored the logging-config branch December 14, 2021 20:55
@Goldziher Goldziher deleted the logging-config branch December 15, 2021 20:02
provinzkraut added a commit that referenced this pull request Nov 11, 2022
Goldziher pushed a commit that referenced this pull request Feb 17, 2023
� This is the 1st commit message:

integrate msgspec into signature modelling

� The commit message #2 will be skipped:

� Add dependency injection of classes (#1143)
�
� * add support for classes in dependency injection
�
� * Apply suggestions from code review
�
� Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
�
� * address review comments
�
� ---------
�
� Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
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.

None yet

3 participants