If a condition fails, the health endpoint crashes with a 500

[dvarrazzo]

If a condition function throws an exception, the health endpoint fails with a 500. I don't think this is the right thing to do for a function supposed to check for something that might be in failing state.

As a consequence, the way I'm using fastapi-health, in an app using Postgres, RabbitMQ, MinIO, looks like:

health_endpoint = health([database_check, rabbitmq_check, storage_check])
app.add_api_route("/health", health_endpoint, include_in_schema=False)

async def database_check() -> bool:
    try:
        async with db.pool.connection(timeout=5) as conn:
            await conn.execute("select 1")
        return True
    except Exception as e:
        logger.error("error checking database: %s - %s", type(e).__name__, e)
        return False

async def rabbitmq_check() -> bool:
    try:
        await mq.mq.check_health()
        return True
    except Exception as e:
        logger.error("error checking rabbitmq: %s - %s", type(e).__name__, e)
        return False

async def storage_check() -> bool:
    try:
        await FileUploader.check_health(minio_url)
        return True
    except Exception as e:
        logger.error("error checking storage: %s - %s", type(e).__name__, e)
        return False

I think that the health endpoint should be just tolerant of failure and consider an error thrown by a condition function as a failure. I should be able to write the above as:

async def database_check() -> bool:
    async with db.pool.connection(timeout=5) as conn:
        await conn.execute("select 1")
    return True

async def rabbitmq_check() -> bool:
    await mq.mq.check_health()
    return True

async def storage_check() -> bool:
    await FileUploader.check_health(minio_url)
    return True

For me the return True is redundant too, but that's probably a different matter.

Thoughts?

[Kludex]

I do imagine people having control on the exception, i.e. doing what you are actually doing, but I see the point.

What do you expect to see in the server logs when there's an exception? fastapi-health should show a custom message with that exception?

Do you think this can be configured, or the default should be what you suggest?

[dvarrazzo]

I think that sending a warning or an error in a logger is something we can do. People can configure the logger as they please to display or not display such errors.

I think that throwing 500 is not a good default. I don't think there should be this choice, so no, I don't see anything to configure.

For me an exception should be treated as a return False is.

[Kludex]

Ok, we can do that then. 👍

Do you want to create a PR for it?

[parthvnp]

@Kludex I don't see any PR for this. Can I work on this?

[Kludex]

Yes

[parthvnp]

@Kludex Before I make a PR, I just want to make sure that the approach I have chosen is a sensible one. To achieve the behaviour @dvarrazzo mentioned, we can create a simple condition_wrapper decorator that will be responsible for calling the condition within a try-except block. Then, if an exception is raised, it returns False else True. Additionally with this approach, we can also log the exception to a logger if the user provides one. Here is the sample condition wrapper:

def condition_wrapper(condition: Callable) -> Callable:
    @wraps(condition)
    async def wrapper(*args, **kwargs) -> bool:
        try:
            if is_async_callable(condition):
                return await condition(*args, **kwargs)
            condition(*args, **kwargs)
            return True
        except (Exception,) as e:
            return False
    return wrapper

Here is how the health function will be modified to decorate the condition:

    params = []
    for condition in conditions:
        wrapped_condition = condition_wrapper(condition)
        params.append(
            Parameter(
                f"{condition.__name__}",
                kind=Parameter.POSITIONAL_OR_KEYWORD,
                annotation=bool,
                default=Depends(wrapped_condition),
            )
        )
    endpoint.__signature__ = Signature(params)
    return endpoint

[Kludex]

Why not return the condition instead of return True?

But that idea works.