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

Control Logging level via ENV variable #107

Closed
IsmailM opened this issue Jul 31, 2020 · 4 comments
Closed

Control Logging level via ENV variable #107

IsmailM opened this issue Jul 31, 2020 · 4 comments
Assignees

Comments

@IsmailM
Copy link
Member

IsmailM commented Jul 31, 2020

Add ENV_LOG_TYPE to handle control levels.

e.g. INFO (or even DEBUG) by default and then ERROR when ENV_LOG_TYPE=production?

Also use this ENV to control the echo value when creating postgres engine

https://github.com/phenopolis/phenopolis_api/blob/e53e1d789efee43eb5f2e085a19adb6bbe9a07f3/views/postgres.py#L28

@alanwilter
Copy link
Collaborator

I've tried to look into this as I need some ENV vars for my attempt to implement coverage.

I've started with something like this in views.py:__ini__.py:

# Options are: prod, dev, debug (default), test (for coverage)
APP_ENV = os.getenv("APP_ENV", "debug")

ENV_LOG_FLAG = True
if APP_ENV in ["test", "prod"]:
    ENV_LOG_FLAG = False

and so I could address postgres.py with:

engine = create_engine(application.config["SQLALCHEMY_DATABASE_URI"], echo=ENV_LOG_FLAG)

So, doing export APP_ENV=test and I can run pytest --cov ... fine.

But now I'm confused because autocomplete.py uses logzero:

...
from logzero import logger
...
    logger.debug("Autocomplete query '%s' and query type '%s'", query, query_type)
...

And everywhere else I see e.g.:

    application.logger.info("Delete session")

With logzero, I think I could do:

if APP_ENV == 'prod':
    # Set a minimum log level
    logzero.loglevel(logging.ERROR)

But how would it work with application.logger? Better yet, why we have this mixed solution? Was it a transition? Should we move to logzero then?

All that said, I have no idea how is it in AWS production servers.

@priesgo
Copy link
Contributor

priesgo commented Sep 7, 2020

I started to put in logzero and then I learned about flask integrated logs so I stopped that. Thus logzero stuff is my legacy (ie: trash), it can be replaced by the application.logger.

In order to configure logs with your variable you need to touch the method _configure_logs() in views/__init__.py as follows:

def _configure_logs():
    application_environment = os.getenv("APP_ENV", "debug")
    log_level = logging.DEBUG if application_environment == "debug" else logging.ERROR
    dictConfig(
        {
            "version": 1,
            "formatters": {
                "default": {"format": "%(asctime)s-%(levelname)s-%(name)s::%(module)s|%(lineno)s:: %(message)s"}
            },
            "handlers": {
                "wsgi": {
                    "class": "logging.StreamHandler",
                    "stream": "ext://flask.logging.wsgi_errors_stream",
                    "formatter": "default",
                },
                "info_rotating_file_handler": {
                    "level": log_level,
                    "formatter": "default",
                    "class": "logging.handlers.RotatingFileHandler",
                    "filename": "phenopolis.log",
                    "mode": "a",
                    "maxBytes": 1048576,
                    "backupCount": 10,
                },
            },
            "root": {"level": log_level, "handlers": ["wsgi"]},
        }
    )
    # add SQLalchemy logs
    logging.getLogger("sqlalchemy").addHandler(default_handler)

Also, I see in your code that you are using all big case variable names. I think the convention is to use that only for constants. Not sure if that is PEP8 or not. While the environment variables are normally all big case, the python variables where you store their value should not. That said I don't care much about this, but the checks in place may complain.

@alanwilter
Copy link
Collaborator

I will go ahead then, retiring logzero and use your suggested solution above. Re UPPER CASE APP_ENV, funny, I see it as a constant rather than a variable, albeit it's used as an arg. I will try to find out what PEP8 says about it yet none of my linters, format checkers etc. had complained about it.

pontikos pushed a commit that referenced this issue Sep 12, 2020
* For using coverage with APP_ENV test

* Typo

* Reorganised

* Pytest to print which APP_ENV is being used

* To make it work with coverage

* Using APP_ENV to define ENV_LOG_FLAG (partially #107)

* Skip F811 check

* Updated with coverage info

* Install process-tests

* Fixed docker-compose arg position

* Use coverage rather test for APP_ENV

* Using gunicorn
@alanwilter
Copy link
Collaborator

I did check https://www.python.org/dev/peps/pep-0008/#constants and the whole thing is, for me, APP_ENV must be seen as a CONSTANT, yet it's defined in the execution command (or defaulted), e.g.:

APP_ENV=coverage pytest --cov views --cov db --cov-report term-missing:skip-covered -sv

it can also be defined by export APP_ENV=prod in AWS env, for instance.

pontikos pushed a commit that referenced this issue Oct 5, 2020
* Removed logzero

* Control Logging level via APP_ENV variable #107
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