build dynamic timeseries_db based on selected backend from registry #733
Conversation
📝 WalkthroughWalkthroughA registry for timeseries backends is added and the backend loading flow is refactored. A new build_timeseries_db() produces a canonical TIMESERIES_DB config (resolving legacy INFLUXDB* settings or the new TIMESERIES_DATABASE dict), validates required keys and OPTIONS, and reports the config source. Backend resolution and loading now use registry utilities (paths, defaults, required keys) and improved ImproperlyConfigured errors. PUBLIC constants expose the default backend path and builtin keys. Sequence Diagram(s)sequenceDiagram
participant Settings
participant ConfigBuilder as _build_timeseries_db()
participant Registry
participant Loader as load_backend_module()
participant BackendModule
participant Client as TimeseriesClient
Settings->>ConfigBuilder: read INFLUXDB_* or TIMESERIES_DATABASE
ConfigBuilder->>Registry: resolve backend path & defaults
Registry-->>ConfigBuilder: default options, required keys
ConfigBuilder->>ConfigBuilder: validate required keys and OPTIONS type
ConfigBuilder-->>Loader: provide resolved backend_path and source
Loader->>Registry: verify builtin vs custom backend
alt builtin backend
Loader->>BackendModule: import builtin backend module
else custom backend
Loader->>BackendModule: import custom module (or raise ImproperlyConfigured)
end
BackendModule-->>Loader: backend module (client class, queries)
Loader->>Client: instantiate client and queries with validated config
Client-->>Settings: Timeseries client ready (exposes .queries)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_monitoring/db/backends/__init__.py`:
- Around line 22-80: The current _build_timeseries_db function treats falsy
values (like {} or []) as "missing" and falls back to legacy INFLUXDB_*
settings; change the presence check from a falsy check to an explicit None check
(i.e., replace "if not raw:" with "if raw is None:") so that empty or invalid
TIMESERIES_DATABASE values are validated later (the subsequent isinstance(raw,
dict) and key checks will then raise ImproperlyConfigured for empty dicts or
invalid types). Ensure the rest of the function still uses raw,
resolve_backend_path(raw["BACKEND"]), and the OPTIONS validation as-is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_monitoring/db/backends/__init__.pyopenwisp_monitoring/db/backends/registry.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_monitoring/db/backends/__init__.py (1)
openwisp_monitoring/db/backends/registry.py (4)
builtin_backend_paths(29-30)default_options_for_backend(33-37)required_keys_for_backend(46-50)resolve_backend_path(40-43)
🪛 Ruff (0.14.14)
openwisp_monitoring/db/backends/__init__.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Avoid specifying long messages outside the exception class
(TRY003)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
111-114: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
🔇 Additional comments (11)
openwisp_monitoring/db/backends/registry.py (6)
5-9: Immutable BackendSpec definition looks good.
12-27: Centralized built‑in backend defaults are clear.
29-30: Helper for built‑in backend paths is straightforward.
33-37: Default options helper returns a defensive copy—nice.
40-43: Backend path resolution is simple and correct.
46-50: Required‑keys lookup is clear and predictable.openwisp_monitoring/db/backends/__init__.py (5)
7-14: Registry integration imports look good.
18-19: Backend constants are clear and intentional.
83-83: TIMESERIES_DB initialization is clean.
86-115: Backend loader flow and validation read well.
118-119: Client/queries wiring is clear and direct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def _build_timeseries_db(): | ||
| """ | ||
| Build a FLAT TIMESERIES_DB dict (for backward compatibility). | ||
|
|
||
| - If TIMESERIES_DATABASE is missing: use legacy INFLUXDB_* settings (deprecated) | ||
| - If TIMESERIES_DATABASE exists: validate/normalize it and resolve BACKEND | ||
| """ | ||
| raw = getattr(settings, "TIMESERIES_DATABASE", None) | ||
|
|
||
| if not raw: | ||
| backend_path = CURRENT_BACKEND | ||
| defaults = default_options_for_backend(backend_path) | ||
| cfg = { | ||
| "BACKEND": backend_path, | ||
| "USER": getattr( | ||
| settings, "INFLUXDB_USER", defaults.get("USER", "openwisp") | ||
| ), | ||
| "PASSWORD": getattr(settings, "INFLUXDB_PASSWORD", "openwisp"), | ||
| "NAME": getattr( | ||
| settings, "INFLUXDB_DATABASE", defaults.get("NAME", "openwisp2") | ||
| ), | ||
| "HOST": getattr( | ||
| settings, "INFLUXDB_HOST", defaults.get("HOST", "localhost") | ||
| ), | ||
| "PORT": getattr(settings, "INFLUXDB_PORT", defaults.get("PORT", "8086")), | ||
| "OPTIONS": {}, | ||
| } | ||
| logger.warning( | ||
| "Timeseries Database definition method deprecated. " | ||
| "Please refer to the docs:\n" | ||
| "https://github.com/openwisp/openwisp-monitoring#setup-integrate-in-an-existing-django-project" | ||
| ) | ||
| return cfg, "legacy INFLUXDB_* settings" | ||
|
|
||
| # New path: TIMESERIES_DATABASE dict | ||
| if not isinstance(raw, dict): | ||
| raise ImproperlyConfigured("TIMESERIES_DATABASE must be a dictionary.") | ||
| if "BACKEND" not in raw: | ||
| raise ImproperlyConfigured('TIMESERIES_DATABASE must define "BACKEND".') | ||
|
|
||
| backend_path = resolve_backend_path(raw["BACKEND"]) | ||
|
|
||
| cfg = {"BACKEND": backend_path} | ||
| cfg.update(default_options_for_backend(backend_path)) | ||
|
|
||
| # Merge user overrides | ||
| for k, v in raw.items(): | ||
| if k not in ("BACKEND", "OPTIONS"): | ||
| cfg[k] = v | ||
|
|
||
| # Normalize OPTIONS | ||
| options = raw.get("OPTIONS") or {} | ||
| if not isinstance(options, dict): | ||
| raise ImproperlyConfigured( | ||
| 'TIMESERIES_DATABASE["OPTIONS"] must be a dictionary.' | ||
| ) | ||
| cfg["OPTIONS"] = options | ||
|
|
||
| return cfg, "settings.TIMESERIES_DATABASE" |
There was a problem hiding this comment.
Treat empty TIMESERIES_DATABASE as misconfiguration, not legacy fallback.
if not raw: treats {} or [] as “missing” and silently falls back to legacy INFLUXDB_* settings, which can mask config errors and contradict the documented behavior. Use an explicit None check so empty/invalid values are handled by the validation below.
✅ Proposed fix
- raw = getattr(settings, "TIMESERIES_DATABASE", None)
-
- if not raw:
+ raw = getattr(settings, "TIMESERIES_DATABASE", None)
+
+ if raw is None:
backend_path = CURRENT_BACKEND
defaults = default_options_for_backend(backend_path)
cfg = {🧰 Tools
🪛 Ruff (0.14.14)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Avoid specifying long messages outside the exception class
(TRY003)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@openwisp_monitoring/db/backends/__init__.py` around lines 22 - 80, The
current _build_timeseries_db function treats falsy values (like {} or []) as
"missing" and falls back to legacy INFLUXDB_* settings; change the presence
check from a falsy check to an explicit None check (i.e., replace "if not raw:"
with "if raw is None:") so that empty or invalid TIMESERIES_DATABASE values are
validated later (the subsequent isinstance(raw, dict) and key checks will then
raise ImproperlyConfigured for empty dicts or invalid types). Ensure the rest of
the function still uses raw, resolve_backend_path(raw["BACKEND"]), and the
OPTIONS validation as-is.
mahmoudnasser1561
force-pushed
the
gsoc/timeseries-backend-registry
branch
from
10ad4e2 to
a9441fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_monitoring/db/backends/__init__.py`:
- Around line 72-78: The code currently uses options = raw.get("OPTIONS") or {}
which masks falsy non-dict values (e.g., "", 0, []) and bypasses the type check;
change the logic to treat an absent OPTIONS key as the only case that gets the
default {}, and otherwise validate the provided value: check if "OPTIONS" in
raw, set options = raw["OPTIONS"], then if not isinstance(options, dict) raise
ImproperlyConfigured('TIMESERIES_DATABASE["OPTIONS"] must be a dictionary.'),
else assign cfg["OPTIONS"] = options; if "OPTIONS" is not in raw assign
cfg["OPTIONS"] = {}.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_monitoring/db/backends/__init__.pyopenwisp_monitoring/db/backends/registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_monitoring/db/backends/registry.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_monitoring/db/backends/__init__.py (1)
openwisp_monitoring/db/backends/registry.py (4)
builtin_backend_paths(29-30)default_options_for_backend(33-37)required_keys_for_backend(46-50)resolve_backend_path(40-43)
🪛 Ruff (0.14.14)
openwisp_monitoring/db/backends/__init__.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Avoid specifying long messages outside the exception class
(TRY003)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
111-114: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (3)
openwisp_monitoring/db/backends/__init__.py (3)
6-20: Good registry wiring and public constants.
Clear exposure ofCURRENT_BACKEND/BUILTIN_BACKEND_KEYSand registry helpers reads well.
86-115: LGTM: validation before import is clean and readable.
118-119: Initialization looks solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Normalize OPTIONS | ||
| options = raw.get("OPTIONS") or {} | ||
| if not isinstance(options, dict): | ||
| raise ImproperlyConfigured( | ||
| 'TIMESERIES_DATABASE["OPTIONS"] must be a dictionary.' | ||
| ) | ||
| cfg["OPTIONS"] = options |
There was a problem hiding this comment.
Validate OPTIONS without masking invalid falsy values.
Line 73 coerces falsy non-dicts (e.g., [], 0, "") to {}, so the type check won’t fire and misconfigurations slip through until runtime.
🛠️ Proposed fix
- options = raw.get("OPTIONS") or {}
- if not isinstance(options, dict):
+ if "OPTIONS" in raw:
+ options = raw["OPTIONS"]
+ if options is None:
+ options = {}
+ else:
+ options = {}
+ if not isinstance(options, dict):
raise ImproperlyConfigured(
'TIMESERIES_DATABASE["OPTIONS"] must be a dictionary.'
)🧰 Tools
🪛 Ruff (0.14.14)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@openwisp_monitoring/db/backends/__init__.py` around lines 72 - 78, The code
currently uses options = raw.get("OPTIONS") or {} which masks falsy non-dict
values (e.g., "", 0, []) and bypasses the type check; change the logic to treat
an absent OPTIONS key as the only case that gets the default {}, and otherwise
validate the provided value: check if "OPTIONS" in raw, set options =
raw["OPTIONS"], then if not isinstance(options, dict) raise
ImproperlyConfigured('TIMESERIES_DATABASE["OPTIONS"] must be a dictionary.'),
else assign cfg["OPTIONS"] = options; if "OPTIONS" is not in raw assign
cfg["OPTIONS"] = {}.
1 participant
Checklist
Reference to Existing Issue
Closes #731
Description of Changes
This PR removes InfluxDB-specific assumptions from the timeseries backend loader and introduces a small built-in backend registry. This makes it possible to add new timeseries backends without modifying the generic loader logic again.
It also paves the way for a dynamic backend option, where Ansible/Docker can choose the backend and
db/backends/__init__.pyloads the correct backend accordingly.Problems addressed
Backend selection was not extensible
The loader implicitly assumed a single backend (InfluxDB) and enforced a fixed set of InfluxDB-specific keys (USER, PASSWORD, HOST, PORT, NAME). This blocks adding additional backends because the loader enforces InfluxDB’s configuration contract globally.
Backend configuration defaults were scattered
Defaults for connection parameters and backend paths were embedded in the module loader logic. As we add support for other backends maintaining backend connection parameters would be hard.