chainlit icon indicating copy to clipboard operation
chainlit copied to clipboard

feat: allow using UVICORN_LOG_CONFIG env var to be used to configure uvicorn logging.

Open LasseGravesenSaxo opened this issue 1 year ago • 6 comments

Motivation

The reason for this PR is that I'm having issues configuring all the loggers, and this causes problems with some log messages being in a different format than expected; We want all our logging to be json such that it can be ingested into our logging aggregator as a single blob, but with the default logging config that cannot be overridden currently we instead sometimes get a bunch of separate log messages that can't be ingested as a single blob and are missing important fields.

Implementation

We let UVICORN_LOG_CONFIG be loaded from the environment, its default value is uvicorn.config.LOGGING_CONFIG (unsure how to handle this best really, either we need a reference to uvicorn.config.LOGGING_CONFIG or it should instead not set the log_config kwarg in the uvicorn.Config if it's not set, which would then fallback to the uvicorn default value; because if we set it to None then it won't use the default logging config). This config value is then passed to uvicorn.Config which is used to configure the uvicorn server.

The value of UVICORN_LOG_CONFIG could be logging.conf, and logging.conf could look something like this:

[loggers]
keys = uvicorn

[logger_uvicorn]
level = DEBUG
handlers = custom
propagate = 0
qualname = uvicorn

[handlers]
keys = custom

[handler_custom]
class = StreamHandler
level = DEBUG
formatter = json
args = (sys.stdout,)

[formatters]
keys = json

[formatter_json]
class = ...

Questions

  1. It might be that this PR should also modify this file: https://github.com/Chainlit/chainlit/blob/main/backend/chainlit/logger.py

To ensure that logging isn't configured multiple times.

  1. It might be that there are other uvicorn configs that should passthru like this. I don't know if we can start uvicorn in such a way that it respects all environment variables without just duplicating what they've done here: https://github.com/encode/uvicorn/blob/master/uvicorn/main.py

LasseGravesenSaxo avatar Apr 04 '24 11:04 LasseGravesenSaxo

Hope someone can take a look at this!

LasseGravesenSaxo avatar Apr 23 '24 10:04 LasseGravesenSaxo

We are also interested in having an option to customize the Uvicorn logging. @LasseGravesenSaxo why did you choose a plain environment variable? For me, it would make a lot more sense to introduce a new CLI option.

Like this:

@click.option(
    "-l",
    "--log-config",
    envvar="LOG_LEVEL",
    help="Uvicorn logging configuration file. Supported formats: .ini, .json, .yaml.",
)
def chainlit_run(target, watch, headless, debug, ci, no_cache, host, port, log_config):
    pass

If the option is not given, the default value of Uvicorn will be used anyway, so no need to define a custom default here.

smueller18 avatar May 24 '24 11:05 smueller18

We are also interested in having an option to customize the Uvicorn logging. @LasseGravesenSaxo why did you choose a plain environment variable? For me, it would make a lot more sense to introduce a new CLI option.

Like this:

@click.option(
    "-l",
    "--log-config",
    envvar="LOG_LEVEL",
    help="Uvicorn logging configuration file. Supported formats: .ini, .json, .yaml.",
)
def chainlit_run(target, watch, headless, debug, ci, no_cache, host, port, log_config):
    pass

If the option is not given, the default value of Uvicorn will be used anyway, so no need to define a custom default here.

@smueller18 LOG_LEVEL and --log-config should be different.

I picked an environment variable because of this bit already using an environment variable for Uvicorn:

    ws_per_message_deflate_env = os.environ.get(
        "UVICORN_WS_PER_MESSAGE_DEFLATE", "true"
    )
    ws_per_message_deflate = ws_per_message_deflate_env.lower() in [
        "true",
        "1",
        "yes",
    ]  # Convert to boolean

But I can easily modify it such that it uses an option too.

LasseGravesenSaxo avatar May 24 '24 11:05 LasseGravesenSaxo

@smueller18, I made a change now that makes it possible to set log config as a click option.

LasseGravesenSaxo avatar May 24 '24 11:05 LasseGravesenSaxo

@smueller18 I applied changes based on your comments.

LasseGravesenSaxo avatar May 24 '24 14:05 LasseGravesenSaxo

Interested in seeing this get attention from maintainers as it's causing an issue with logging for us.

Atheuz avatar Jun 28 '24 22:06 Atheuz

Hey all, we're finally getting to give love to some of these long-pending contribs.

As in the meantime, we've landed the possibility to mount chainlit in FastAPI apps, so that we can run and configure our own uvicorn, is this still a relevant and necessary feature?

I'm asking as I'd imagine that anyone running chainlit in production will want to do so with their own uvicorn, where they have full control over it's config.

This way, we won't end up having to maintain wrappers for the plethora of uvicorn options without blocking devs accesss to lower-level stuff. The same goes for FastAPI.

In this perspective, the chainlit CLI is merely a wrapper to land quick PoC's and a full-fledged ASGI would be the production-grade wrapper for scale-out (similar to how e.g. Django does it).

Love to hear your feedback @LasseGravesenSaxo and @smueller18.

dokterbob avatar Aug 22 '24 11:08 dokterbob

@dokterbob The option for mounting chainlit in a custom FastAPI app was a really nice feature and we migrated to. So for us, there is no need for this specific PR anymore.

smueller18 avatar Aug 23 '24 12:08 smueller18

@smueller18 Thanks for the head's up. I will close this one for now.

dokterbob avatar Aug 23 '24 12:08 dokterbob

We are good also now using what smueller18 said.

LasseGravesenSaxo avatar Aug 28 '24 14:08 LasseGravesenSaxo