Skip to content

Conversation

@alxtkr77
Copy link
Member

@alxtkr77 alxtkr77 commented Dec 4, 2025

TITLE: [Model Monitoring] Add TimescaleDB database auto-creation

Summary

Add automatic database creation for TimescaleDB connector. When auto_create_database is enabled (default), the connector generates a database name using system_id and creates it if it does not exist.

Changes Made

  • Add tsdb.auto_create_database config option to mlrun/config.py (default: True)
  • Add admin_dsn() method to DatastoreProfilePostgreSQL for connecting to postgres maintenance database
  • Add dsn(database=...) parameter to override database in DSN
  • Add _determine_database_name() to TimescaleDBConnector
  • Add _create_db_if_not_exists() to TimescaleDBOperationsManager
  • Add unit tests for database auto-creation functionality (4 tests)
  • Add unit tests for admin_dsn() and dsn() methods (2 tests)

Testing

  • 107/107 TimescaleDB tests pass
  • 5/5 PostgreSQL datastore profile tests pass
  • Tested database creation, idempotency, and error handling

Checklist

  • Code follows project style guidelines
  • Tests added for new functionality
  • All tests pass locally
  • Documentation updated (internal docstrings)

Reference

Add automatic database creation for TimescaleDB connector. When
`auto_create_database` is enabled (default), the connector generates
a database name using `system_id` and creates it if it doesn't exist.

Changes:
- Add `tsdb.auto_create_database` config option (default: True)
- Add `admin_dsn()` method to `DatastoreProfilePostgreSQL`
- Add `dsn(database=...)` parameter override
- Add `_determine_database_name()` to `TimescaleDBConnector`
- Add `_create_db_if_not_exists()` to `TimescaleDBOperationsManager`
- Add unit tests for database auto-creation functionality
Copy link
Collaborator

@gtopper gtopper left a comment

Choose a reason for hiding this comment

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

LGTM by and large, but had some comments regarding semantics, style, logs, a minor input sanitation concern, and at least one place where we might need to close the connection pool.

alxtkr77 pushed a commit to alxtkr77/mlrun that referenced this pull request Dec 15, 2025
Changes based on PR mlrun#9012 review:

1. URL encoding consistency in DatastoreProfiles:
   - Add URL encoding for user/password in dsn() methods
   - Add URL decoding in from_dsn() methods (urlparse doesn't decode)
   - Fix affects PostgreSQL, TDEngine, and Redis profiles
   - Add comprehensive tests for URL encoding round-trips

2. Terminology improvements:
   - Replace "maintenance database" with "default postgres database"
   - Replace "target database" with "monitoring database"

3. TimescaleDBConnection improvements:
   - Add close() method for proper encapsulation
   - Update timescaledb_operations to use close() instead of _pool.close()
alxtkr77 pushed a commit to alxtkr77/mlrun that referenced this pull request Dec 15, 2025
Changes based on PR mlrun#9012 review:

1. URL encoding consistency in DatastoreProfiles:
   - Add URL encoding for user/password in dsn() methods
   - Add URL decoding in from_dsn() methods (urlparse doesn't decode)
   - Fix affects PostgreSQL, TDEngine, and Redis profiles
   - Add comprehensive tests for URL encoding round-trips

2. Terminology improvements:
   - Replace "maintenance database" with "default postgres database"
   - Replace "target database" with "monitoring database"

3. TimescaleDBConnection improvements:
   - Add close() method for proper encapsulation
   - Update timescaledb_operations to use close() instead of _pool.close()
alxtkr77 pushed a commit to alxtkr77/mlrun that referenced this pull request Dec 15, 2025
Changes based on PR mlrun#9012 review:

1. URL encoding consistency in DatastoreProfiles:
   - Add URL encoding for user/password in dsn() methods
   - Add URL decoding in from_dsn() methods (urlparse doesn't decode)
   - Fix affects PostgreSQL, TDEngine, and Redis profiles
   - Add comprehensive tests for URL encoding round-trips

2. Terminology improvements:
   - Replace "maintenance database" with "default postgres database"
   - Replace "target database" with "monitoring database"

3. TimescaleDBConnection improvements:
   - Add close() method for proper encapsulation
   - Update timescaledb_operations to use close() instead of _pool.close()
Copy link
Collaborator

@gtopper gtopper left a comment

Choose a reason for hiding this comment

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

Other than that, LGTM ✅

alxtkr77 pushed a commit to alxtkr77/mlrun that referenced this pull request Dec 16, 2025
Changes based on PR mlrun#9012 review:

1. URL encoding consistency in DatastoreProfiles:
   - Add URL encoding for user/password in dsn() methods
   - Add URL decoding in from_dsn() methods (urlparse doesn't decode)
   - Fix affects PostgreSQL, TDEngine, and Redis profiles
   - Add comprehensive tests for URL encoding round-trips

2. Terminology improvements:
   - Replace "maintenance database" with "default postgres database"
   - Replace "target database" with "monitoring database"

3. TimescaleDBConnection improvements:
   - Add close() method for proper encapsulation
   - Update timescaledb_operations to use close() instead of _pool.close()
Changes based on PR mlrun#9012 review:

1. URL encoding consistency in DatastoreProfiles:
   - Add URL encoding for user/password in dsn() methods
   - Add URL decoding in from_dsn() methods (urlparse doesn't decode)
   - Fix affects PostgreSQL, TDEngine, and Redis profiles
   - Add comprehensive tests for URL encoding round-trips

2. Terminology improvements:
   - Replace "maintenance database" with "default postgres database"
   - Replace "target database" with "monitoring database"

3. TimescaleDBConnection improvements:
   - Add close() method for proper encapsulation
   - Update timescaledb_operations to use close() instead of _pool.close()
@gtopper gtopper merged commit a20a595 into mlrun:development Dec 17, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants