Skip to content

Commit a7c423e

Browse files
mchadesCopilot
andauthored
[#70] feat(logging): add YAML-based file logging and key-path log coverage (#71)
### What changes were proposed in this pull request? - Add `conf/logging_conf.yaml.template`: Python `dictConfig` YAML with `console` (stderr) + `RotatingFileHandler` at `./hypervisor-logs/hypervisor.log` (10 MB, 5 backups) - Rewrite `_configure_logging()` in `__main__.py`: loads YAML from config dir, auto-creates log directories, prints startup notice to stderr before logger init; `--log-level` now overrides config-file level instead of being the sole source - Fill key-path log gaps: - `protocol/dispatcher.py`: per-request `method`, `request_id`, `status`, `duration_ms` - `policy/enforcer.py`: WARNING on access-denied, DEBUG on access-granted and role resolution - `handlers/ping.py`, `handlers/initialize.py`, `handlers/execute.py`: lifecycle logs - `manifest/yaml_provider.py`: `backends=N, resources=N` counts on load - `backends/rdbms/backend.py`: query completion with timing and row count - Update README with Logging Configuration section ### Why are the changes needed? Fix: #70 The server had no file logging and several critical paths produced no log output, making it difficult to diagnose issues in deployed environments. ### Does this PR introduce _any_ user-facing change? - New CLI default behavior: logs are now written to `./hypervisor-logs/hypervisor.log` (relative to CWD) in addition to stderr. The directory is created automatically. - `--log-level` now overrides the config-file level rather than being the sole source of truth. - New file `conf/logging_conf.yaml.template` can be copied to the config directory as `logging_conf.yaml` to customise log format, rotation, and level. ### How was this patch tested? - All 624 unit tests pass (`uv run python -m unittest discover -s tests -v`) - `uv run black . && uv run ruff check . && uv run mypy` — all clean - Live server smoke test with localfs backend: confirmed `hypervisor-logs/hypervisor.log` is created in CWD, stdout contains only JSON-RPC responses, all key-path logs appear in both stderr and file (startup, manifest load, backend connect, request dispatch with timing, policy role resolution, execute with row count, graceful shutdown) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8e50904 commit a7c423e

File tree

10 files changed

+313
-27
lines changed

10 files changed

+313
-27
lines changed

README.md

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,41 @@ The server starts in stdio mode, reading JSON-RPC requests from stdin and writin
7070
python -m adp_hypervisor --config <path> [--log-level LEVEL] [--transport TYPE]
7171
```
7272

73-
| Option | Default | Description |
74-
|:--------------|:---------|:-----------------------------------------------------------------------|
75-
| `--config` | Required | Path to manifest directory (physical.yaml, semantic.yaml, policy.yaml) |
76-
| `--log-level` | `INFO` | Logging level: DEBUG, INFO, WARNING, ERROR, CRITICAL |
77-
| `--transport` | `stdio` | Transport type: `stdio` (HTTP planned for future release) |
73+
| Option | Default | Description |
74+
|:--------------|:-------------------|:-----------------------------------------------------------------------|
75+
| `--config` | Required | Path to manifest directory (physical.yaml, semantic.yaml, policy.yaml) |
76+
| `--log-level` | From logging config | Override root log level: DEBUG, INFO, WARNING, ERROR, CRITICAL |
77+
| `--transport` | `stdio` | Transport type: `stdio` (HTTP planned for future release) |
78+
79+
### Logging Configuration
80+
81+
Logging is configured via `logging_conf.yaml` in the config directory. The file follows
82+
Python's standard `logging.config.dictConfig` format.
83+
84+
Copy the provided template to get started:
85+
86+
```bash
87+
cp conf/logging_conf.yaml.template <config-dir>/logging_conf.yaml
88+
```
89+
90+
**Default behaviour (used when no `logging_conf.yaml` is present):**
91+
92+
- Logs go to both `stderr` (console) and a rotating file at `./hypervisor-logs/hypervisor.log`
93+
- The log directory is created automatically if it does not exist
94+
- The resolved log file path is printed to `stderr` at startup
95+
- Root log level: `INFO`
96+
- File rotation: 10 MB per file, 5 backup files
97+
98+
**Key tunable fields in `logging_conf.yaml`:**
99+
100+
| Field | Default | Description |
101+
|:------|:--------|:------------|
102+
| `root.level` | `INFO` | Root log level (overridable with `--log-level`) |
103+
| `handlers.file_handler.filename` | `./hypervisor-logs/hypervisor.log` | Log file path (relative to CWD) |
104+
| `handlers.file_handler.maxBytes` | `10485760` (10 MB) | Max file size before rotation |
105+
| `handlers.file_handler.backupCount` | `5` | Number of backup files to keep |
106+
107+
> **Note:** Logs must never go to `stdout` when using stdio transport — that stream is reserved for JSON-RPC responses.
78108
79109
### 3. Send a Request
80110

@@ -153,7 +183,7 @@ uv run mypy src/
153183
```
154184
adp-hypervisor/
155185
├── pyproject.toml # Project configuration
156-
├── conf/ # Manifest templates
186+
├── conf/ # Manifest templates (physical, semantic, policy, logging)
157187
├── examples/ # Ready-to-run demo (Docker + manifests)
158188
├── src/adp_hypervisor/ # Main hypervisor package
159189
│ ├── server.py # ADPServer main class

conf/logging_conf.yaml.template

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Copyright 2026 Datastrato, Inc.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
# ADP Hypervisor Logging Configuration
16+
#
17+
# This file configures the Python logging system via logging.config.dictConfig.
18+
# Copy this template to your runtime config directory as logging_conf.yaml and
19+
# adjust the values to suit your environment.
20+
#
21+
# Usage:
22+
# cp conf/logging_conf.yaml.template <config-dir>/logging_conf.yaml
23+
#
24+
# The log level can also be overridden at startup with --log-level.
25+
26+
version: 1
27+
disable_existing_loggers: false
28+
29+
formatters:
30+
standard:
31+
format: "%(asctime)s [%(levelname)s] %(name)s: %(message)s"
32+
datefmt: "%Y-%m-%dT%H:%M:%S"
33+
34+
handlers:
35+
console:
36+
class: logging.StreamHandler
37+
formatter: standard
38+
stream: ext://sys.stderr
39+
40+
file_handler:
41+
class: logging.handlers.RotatingFileHandler
42+
formatter: standard
43+
filename: ./hypervisor-logs/hypervisor.log
44+
maxBytes: 10485760 # 10 MB
45+
backupCount: 5
46+
encoding: utf-8
47+
48+
root:
49+
level: INFO
50+
handlers:
51+
- console
52+
- file_handler

src/adp_hypervisor/__main__.py

Lines changed: 116 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,13 @@
2828
import argparse
2929
import asyncio
3030
import logging
31+
import logging.config
32+
import logging.handlers
3133
import sys
3234
from pathlib import Path
35+
from typing import Any
36+
37+
import yaml
3338

3439
from adp_hypervisor.manifest.yaml_provider import YamlManifestProvider
3540
from adp_hypervisor.policy import UserRoleConfig, YamlRoleResolver
@@ -41,6 +46,38 @@
4146
_VALID_LOG_LEVELS = ("DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL")
4247
_VALID_TRANSPORTS = ("stdio", "http")
4348

49+
# Built-in default logging config, matches conf/logging_conf.yaml.template.
50+
# Used when no logging_conf.yaml is present in the config directory.
51+
_DEFAULT_LOGGING_CONFIG: dict[str, Any] = {
52+
"version": 1,
53+
"disable_existing_loggers": False,
54+
"formatters": {
55+
"standard": {
56+
"format": "%(asctime)s [%(levelname)s] %(name)s: %(message)s",
57+
"datefmt": "%Y-%m-%dT%H:%M:%S",
58+
}
59+
},
60+
"handlers": {
61+
"console": {
62+
"class": "logging.StreamHandler",
63+
"formatter": "standard",
64+
"stream": "ext://sys.stderr",
65+
},
66+
"file_handler": {
67+
"class": "logging.handlers.RotatingFileHandler",
68+
"formatter": "standard",
69+
"filename": "./hypervisor-logs/hypervisor.log",
70+
"maxBytes": 10485760,
71+
"backupCount": 5,
72+
"encoding": "utf-8",
73+
},
74+
},
75+
"root": {
76+
"level": "INFO",
77+
"handlers": ["console", "file_handler"],
78+
},
79+
}
80+
4481

4582
def _build_parser() -> argparse.ArgumentParser:
4683
"""Build the CLI argument parser."""
@@ -56,9 +93,9 @@ def _build_parser() -> argparse.ArgumentParser:
5693
)
5794
parser.add_argument(
5895
"--log-level",
59-
default="INFO",
96+
default=None,
6097
choices=_VALID_LOG_LEVELS,
61-
help="Logging level (default: INFO)",
98+
help="Override the root logging level (default: level from logging_conf.yaml or INFO)",
6299
)
63100
parser.add_argument(
64101
"--transport",
@@ -69,13 +106,78 @@ def _build_parser() -> argparse.ArgumentParser:
69106
return parser
70107

71108

72-
def _configure_logging(level: str) -> None:
73-
"""Configure root logging with a consistent format."""
74-
logging.basicConfig(
75-
level=getattr(logging, level),
76-
format="%(asctime)s [%(levelname)s] %(name)s: %(message)s",
77-
stream=sys.stderr,
78-
)
109+
def _load_logging_config(config_dir: Path) -> dict[str, Any]:
110+
"""Load logging configuration from config_dir/logging_conf.yaml.
111+
112+
Falls back to the built-in default when the file is absent.
113+
114+
Args:
115+
config_dir: The runtime config directory.
116+
117+
Returns:
118+
A dict suitable for logging.config.dictConfig.
119+
"""
120+
logging_conf_path = config_dir / "logging_conf.yaml"
121+
if logging_conf_path.exists():
122+
try:
123+
raw = yaml.safe_load(logging_conf_path.read_text(encoding="utf-8"))
124+
if isinstance(raw, dict):
125+
return raw
126+
print(
127+
f"Warning: {logging_conf_path} is not a YAML mapping (got {type(raw).__name__}), "
128+
"using built-in logging defaults.",
129+
file=sys.stderr,
130+
)
131+
except (yaml.YAMLError, OSError):
132+
print(
133+
f"Warning: failed to parse {logging_conf_path}, using built-in logging defaults.",
134+
file=sys.stderr,
135+
)
136+
return _DEFAULT_LOGGING_CONFIG
137+
138+
139+
def _ensure_log_directories(config: dict[str, Any]) -> None:
140+
"""Create parent directories for all file-based log handlers.
141+
142+
Scans every handler in the config dict, resolves the parent directory
143+
for any handler that has a ``filename`` key, and creates it if missing.
144+
Emits a startup notice to stderr for each resolved log file path.
145+
146+
Args:
147+
config: The logging config dict (as passed to dictConfig).
148+
"""
149+
handlers = config.get("handlers", {})
150+
for _name, handler_cfg in handlers.items():
151+
filename = handler_cfg.get("filename")
152+
if not filename:
153+
continue
154+
log_path = Path(filename).resolve()
155+
log_dir = log_path.parent
156+
log_dir.mkdir(parents=True, exist_ok=True)
157+
print(
158+
f"ADP Hypervisor: logging to file {log_path}",
159+
file=sys.stderr,
160+
)
161+
162+
163+
def _configure_logging(config_dir: Path, level_override: str | None) -> None:
164+
"""Load and apply logging configuration.
165+
166+
Reads logging_conf.yaml from config_dir (falls back to built-in defaults),
167+
ensures all file-handler directories exist, then applies the config via
168+
dictConfig. Overrides the root log level when --log-level is provided.
169+
170+
Args:
171+
config_dir: The runtime config directory.
172+
level_override: Optional log level string (e.g. "DEBUG") from --log-level.
173+
"""
174+
config = _load_logging_config(config_dir)
175+
176+
if level_override is not None:
177+
config.setdefault("root", {})["level"] = level_override
178+
179+
_ensure_log_directories(config)
180+
logging.config.dictConfig(config)
79181

80182

81183
def _create_yaml_provider(config_dir: Path) -> YamlManifestProvider:
@@ -161,22 +263,23 @@ def main(args: list[str] | None = None) -> None:
161263
parser = _build_parser()
162264
parsed = parser.parse_args(args)
163265

164-
_configure_logging(parsed.log_level)
266+
config_dir = Path(parsed.config)
267+
_configure_logging(config_dir, parsed.log_level)
165268

166269
if parsed.transport == "http":
167270
logger.error("HTTP transport is not yet implemented")
168271
sys.exit(1)
169272

170273
transport = StdioTransport()
171-
provider = _create_yaml_provider(Path(parsed.config))
172-
role_resolver = _create_role_resolver(Path(parsed.config))
274+
provider = _create_yaml_provider(config_dir)
275+
role_resolver = _create_role_resolver(config_dir)
173276
server = ADPServer(manifest_provider=provider, transport=transport, role_resolver=role_resolver)
174277

175278
logger.info(
176279
"Starting ADP Hypervisor: config=%s, transport=%s, log_level=%s",
177280
parsed.config,
178281
parsed.transport,
179-
parsed.log_level,
282+
parsed.log_level or "from config",
180283
)
181284

182285
asyncio.run(server.run())

src/adp_hypervisor/handlers/execute.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,12 @@ async def handle(self, params: dict[str, Any]) -> BaseModel:
9898
request = ExecuteRequestParams.model_validate(params)
9999

100100
resource_id = request.intent.resource_id
101+
logger.debug(
102+
"Execute: started resource=%s, intent_class=%s",
103+
resource_id,
104+
request.intent.intent_class,
105+
)
106+
101107
resource = self._manifest_index.get_resource(resource_id)
102108
if resource is None:
103109
raise ResourceNotFoundError(f"Resource not found: {resource_id!r}")
@@ -134,8 +140,9 @@ async def handle(self, params: dict[str, Any]) -> BaseModel:
134140
# based on the result set and pass it back via next_cursor.
135141

136142
logger.info(
137-
"Execute: resource=%s, intent_class=%s, rows=%d, duration_ms=%d",
143+
"Execute: resource=%s, backend=%s, intent_class=%s, rows=%d, duration_ms=%d",
138144
resource_id,
145+
resource.backend_id,
139146
request.intent.intent_class,
140147
len(result.rows),
141148
duration_ms,

src/adp_hypervisor/handlers/initialize.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ async def handle(self, params: dict[str, Any]) -> BaseModel:
9494
"""
9595
request = InitializeRequestParams.model_validate(params)
9696

97-
logger.info(
97+
logger.debug(
9898
"Initialize request from %s/%s, protocol version: %s",
9999
request.client_info.name,
100100
request.client_info.version,
@@ -108,6 +108,13 @@ async def handle(self, params: dict[str, Any]) -> BaseModel:
108108
f"Supported versions: {supported}"
109109
)
110110

111+
logger.info(
112+
"Initialize: client=%s/%s, protocol_version=%s accepted",
113+
request.client_info.name,
114+
request.client_info.version,
115+
request.protocol_version,
116+
)
117+
111118
return InitializeResult(
112119
protocol_version=request.protocol_version,
113120
capabilities=self._capabilities,

src/adp_hypervisor/handlers/ping.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,16 @@
1818
Implements the adp.ping method which provides a simple health check.
1919
"""
2020

21+
import logging
2122
from typing import Any
2223

2324
from pydantic import BaseModel
2425

2526
from adp_hypervisor.handlers.base import Handler
2627
from adp_hypervisor.protocol.types import EmptyResult
2728

29+
logger = logging.getLogger(__name__)
30+
2831

2932
class PingHandler(Handler):
3033
"""Handler for the adp.ping method.
@@ -46,4 +49,5 @@ async def handle(self, params: dict[str, Any]) -> BaseModel:
4649
Returns:
4750
An empty result indicating the server is alive.
4851
"""
52+
logger.debug("Ping received")
4953
return EmptyResult()

src/adp_hypervisor/manifest/yaml_provider.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,11 @@ def load(self) -> None:
6060
self._physical = self._load_physical()
6161
self._semantic = self._load_semantic()
6262
self._policy = self._load_policy()
63-
logger.info("Manifests loaded successfully")
63+
logger.info(
64+
"Manifests loaded: backends=%d, resources=%d",
65+
len(self._physical.backends),
66+
len(self._semantic.resources),
67+
)
6468

6569
def get_physical_manifest(self) -> PhysicalManifest:
6670
self._ensure_loaded()
@@ -88,6 +92,7 @@ def _load_yaml(self, path: Path) -> dict: # type: ignore[type-arg]
8892
data = yaml.safe_load(f)
8993
if not isinstance(data, dict):
9094
raise ValueError(f"Expected a YAML mapping in {path}, got {type(data).__name__}")
95+
logger.debug("Loaded YAML: %s (%d top-level keys)", path, len(data))
9196
return data
9297

9398
def _load_physical(self) -> PhysicalManifest:

0 commit comments

Comments
 (0)