⚗️Introduce asynchronous logging facilities (🚨) #8064
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8064 +/- ##
==========================================
- Coverage 88.26% 88.19% -0.08%
==========================================
Files 1879 1878 -1
Lines 72085 72242 +157
Branches 1269 1272 +3
==========================================
+ Hits 63624 63712 +88
- Misses 8094 8155 +61
- Partials 367 375 +8
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
sanderegg
force-pushed
the
maintenance/async-logger-facilities
branch
4 times, most recently
from
40ee833 to
476d08d
Compare
sanderegg
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces non-blocking asynchronous logging support across services by replacing the old synchronous config_all_loggers approach with a new setup_loggers function and an async_loggers_lifespan context manager. It also updates entrypoint scripts to surface the UV binary path and bumps PostgreSQL images in compose files.
- Refactor logging setup: replace
config_all_loggerswithsetup_loggersand offer an async logging lifespan - Add tests for async logging utilities (
async_loggers_lifespan) - Update Docker entrypoints to echo the UV executable and bump Postgres image versions
Reviewed Changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| services/web/server/src/simcore_service_webserver/log.py | Removed sync logger config and updated setup_logging signature |
| services/web/server/src/simcore_service_webserver/cli.py | Added async_loggers_lifespan integration via AsyncExitStack |
| packages/service-library/src/servicelib/logging_utils.py | Implemented setup_loggers and async_loggers_lifespan, deprecated old config function |
| packages/service-library/tests/test_logging_utils.py | Added async logging tests, including retry-based assertions |
| services/*/docker/entrypoint.sh (multiple services) | Echo UV path in entrypoint scripts |
| services/*/docker-compose-extra.yml (Postgres services) | Bumped Postgres images from 14.5 to 14.8 in compose files |
Comments suppressed due to low confidence (3)
services/web/server/docker/entrypoint.sh:30
- [nitpick] The label "UV" is ambiguous and the
uvexecutable may not exist. Consider renaming to "uvicorn" and usingcommand -v uvicornto verify the correct binary.
echo "$INFO" "UV : $(command -v uv)"
services/director-v2/docker-compose-extra.yml:3
- [nitpick] The image tag is wrapped in quotes while others are unquoted. Remove the surrounding quotes for consistency across compose files.
image: "postgres:14.8-alpine@sha256:150dd39ccb7ae6c7ba6130c3582c39a30bb5d3d22cb08ad0ba37001e3f829abc"
packages/service-library/tests/test_logging_utils.py:351
- The use of parentheses around multiple context managers will be interpreted as a single tuple context and raise a syntax error. Change to
with suppress(ValueError), log_exceptions(...)without the outer parentheses.
with (
9186396 to
77f8894
Compare
sanderegg
changed the title
sanderegg
requested review from
GitHK,
YuryHrytsuk,
giancarloromeo,
matusdrobuliak66,
mrnicegyu11 and
pcrespov
as code owners
|
@mergify queue |
🟠 Waiting for conditions to match
|
packages/service-library/src/servicelib/fastapi/logging_lifespan.py
Outdated
Show resolved
Hide resolved
| return _logging_lifespan | ||
|
|
||
|
|
||
| def setup_logging_shutdown_event( |
There was a problem hiding this comment.
We really do not care when the action is taken, the function should not reflect this. Others don't
| def setup_logging_shutdown_event( | |
| def setup_logging( |
There was a problem hiding this comment.
@GitHK please read the documentation of that helper function.
It is used for old style fastapi applications and is to be used as a shutdown event.
Since we have things half way done, with some stuff going to lifespans, some others not as usual, there is a need for the 2 systems.. That does not make things simpler.
Also I don't get your point, why is the other one supposed to be _lifespan suffix and not this one? this makes no sense.
There was a problem hiding this comment.
Ahh yes, I see what you mean.
I would maybe call this function get_setup_logging_shutdown_event since it sounds better to me.
All good though.
sanderegg
force-pushed
the
maintenance/async-logger-facilities
branch
from
b649bb9 to
fae4400
Compare
|
6 participants
What do these changes do?
This very noisy PR introduces asynchronous logging facilities into our services (with the exceptions of the dask-sidecar and storage workers).
Why?
Since our services are heavily relying on asynchronous python code via asyncio, the default python logging that we were using is synchronous, which blocks the event loop everytime a log it written into
stderr.Here are some references about that fact:
How is it done?
logging_utils.pyin serviceliblogging_utils.pywas refactored and now the following happens:logging_utils.pyas well --> reduced copy/pastingfastapi/logging_lifespan.pyin servicelib which brings asetup_logging_lifespanand a backwards-compatiblesetup_logging_shutdown_eventmethodlog.pyandcli.pyin thewebserverserviceTesting
preserve_caplog_for_async_loggingwhich is in autouse mode. Pytest normally attaches special log handlers when tests are running and these are removed by the reworkedlogging_utils. This fixture ensures that the handlers are added again.Additional changes
boot.shfor fastapi-based application: now uses an application factory (there is no real reason for this, but it aligns better with the web server)entrypoint.shnow also print if UV is installedSTORAGE_TRACINGENV variable was missing fromdocker-compose.ymlRelated issue/s
How to test
Dev-ops