Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented Aug 15, 2025

What do these changes do?

This PR ensures that all DB clients are registering themselves with the application name, and a suffix as to which library is used to connect (e.g. aiopg or asyncpg). These client names are visible in the process list in adminer for example.

Typical connection is APP_NAME-HOST_NAME-TASK_SLOT-TYPE

image

Related issue/s

How to test

Dev-ops

@sanderegg sanderegg added this to the Voyager milestone Aug 15, 2025
@sanderegg sanderegg self-assigned this Aug 15, 2025
@sanderegg sanderegg added the a:services-library issues on packages/service-libs label Aug 15, 2025
@codecov
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 82.81250% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.02%. Comparing base (2037e74) to head (12a8d97).
⚠️ Report is 32 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8220      +/-   ##
==========================================
- Coverage   88.08%   88.02%   -0.07%     
==========================================
  Files        1910     1522     -388     
  Lines       73492    61803   -11689     
  Branches     1301      688     -613     
==========================================
- Hits        64734    54399   -10335     
+ Misses       8371     7162    -1209     
+ Partials      387      242     -145     
Flag Coverage Δ
integrationtests 64.20% <84.21%> (+0.01%) ⬆️
unittests 86.39% <75.00%> (-0.33%) ⬇️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.72% <16.66%> (+0.07%) ⬆️
pkg_settings_library 90.17% <100.00%> (-0.29%) ⬇️
pkg_simcore_sdk 85.03% <100.00%> (-0.08%) ⬇️
agent 93.81% <ø> (ø)
api_server 93.26% <83.33%> (+<0.01%) ⬆️
autoscaling 95.89% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.81% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.14% <ø> (ø)
director_v2 90.95% <100.00%> (+0.04%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.08% <70.58%> (-0.04%) ⬇️
efs_guardian 89.62% <100.00%> (+0.01%) ⬆️
invitations 91.44% <ø> (ø)
payments 92.61% <100.00%> (+<0.01%) ⬆️
resource_usage_tracker 92.18% <100.00%> (-0.05%) ⬇️
storage 86.42% <100.00%> (-0.21%) ⬇️
webclient ∅ <ø> (∅)
webserver 88.27% <100.00%> (+<0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2037e74...12a8d97. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sanderegg sanderegg added a:webserver webserver's codebase. Assigning the area is particularly useful for bugs a:director-v2 issue related with the director-v2 service a:payments area: payments service a:efs-guardian labels Aug 15, 2025
@sanderegg sanderegg marked this pull request as ready for review August 15, 2025 07:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR standardizes database client naming conventions across all microservices by ensuring that every DB connection includes a descriptive application name and database driver suffix. The naming convention follows the pattern APP_NAME-HOST_NAME-TASK_SLOT-TYPE, which improves observability by making database connections easily identifiable in admin tools like Adminer.

Key changes include:

  • Updating all database connection methods to accept and use application_name parameter
  • Modifying PostgresSettings to generate client names with configurable suffixes
  • Ensuring backward compatibility while making application names mandatory for new connections

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/settings-library/src/settings_library/postgres.py Enhanced PostgresSettings with new methods for generating client names with suffixes
packages/service-library/src/servicelib/db_asyncpg_utils.py Updated async engine creation functions to require and use application names
packages/service-library/src/servicelib/fastapi/db_asyncpg_engine.py Modified FastAPI DB connection to pass application names through
packages/simcore-sdk/src/simcore_sdk/node_ports_common/dbmanager.py Updated DBManager and DBContextManager to require application names
services/*/src/*/modules/db/*.py Updated all service DB modules to provide APP_NAME when connecting
services/*/src/*/*.py Various service files updated to pass application names to DB connections
Comments suppressed due to low confidence (1)

services/director-v2/tests/integration/02/test_dynamic_sidecar_nodeports_integration.py:36

  • The removal of the unused import ProjectIDStr is good, but this change appears unrelated to the main purpose of this PR which is about database client naming.
    ProjectID,

@mergify
Copy link
Contributor

mergify bot commented Aug 15, 2025

🧪 CI Insights

Here's what we observed from your CI run for 12a8d97.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on base branch Retries 🔍 CI Insights 📄 Logs
CI [sys] e2e (3.11, 14, ubuntu-24.04) Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀 Healthy 1 View View
[unit] storage (3.11, ubuntu-24.04) Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
system-tests Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀 Healthy 1 View View
unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

thx

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks. I suggest to use a symbol different from - to separate the different components of the name because it seems we already use - within our names a lot.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Thanks, just a minor thing

@sonarqubecloud
Copy link

@sanderegg sanderegg merged commit cbf29d6 into ITISFoundation:master Aug 15, 2025
145 of 148 checks passed
@sanderegg sanderegg deleted the db/ensure-app-name branch August 15, 2025 14:58
GitHK pushed a commit to GitHK/osparc-simcore-forked that referenced this pull request Aug 18, 2025
@sanderegg sanderegg added the t:maintenance Some planned maintenance work label Aug 26, 2025
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 2, 2025
61 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:director-v2 issue related with the director-v2 service a:efs-guardian a:payments area: payments service a:services-library issues on packages/service-libs a:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants