Skip to content

fix(observability): shared library integration + health-check fixes#237

Open
roman1887 wants to merge 13 commits into
mainfrom
fix/observability-health-and-shared-lib
Open

fix(observability): shared library integration + health-check fixes#237
roman1887 wants to merge 13 commits into
mainfrom
fix/observability-health-and-shared-lib

Conversation

@roman1887

Copy link
Copy Markdown
Collaborator

What & why

Checks in the observability work + the three health-check bug fixes that made the full stack report healthy. Emit-only model — services emit logs/metrics, no backend bundled.

Fixes (all surfaced by the aggregated /health)

  • Module rename shared/observability/python/logging.pystructured_logging.py — the old name shadowed Python's stdlib logging, causing a circular-import crash on startup.
  • schema-service import — routed through its local app/observability_init.py importlib loader; from shared.observability import ... failed to resolve in-container.
  • base.py httpx — replaced removed private httpx._utils.default_timer() with stdlib time.perf_counter(); the Nexus→schema-service health check was crashing.
  • cache health — memory cache get_stats() now returns "connected": True; an empty in-process cache was wrongly reported unhealthy.
  • llm healthhealth_check() checks the initialized client instead of non-existent attributes.
  • Docker — Dockerfiles + compose build contexts copy the shared library into each image.

Verified

All services healthy through Nexus aggregated /health:

OVERALL: healthy
  schema-service  healthy=True
  llm_service     healthy=True
  trino-service   healthy=True
  cache-service   healthy=True

Docs

README, docker/README, OBSERVABILITY_COMPLETE refreshed with emit-only scope + verified status. (Marketing monitoring.md doc edit lives in the marketing repo, tracked separately.)

Related: #211 (versioning), end-user observability roadmap #210.

🤖 Generated with Claude Code

roman1887 and others added 11 commits June 15, 2026 00:06
…rvices

Implemented comprehensive observability infrastructure:

**Shared Observability Library** (`shared/observability/`)
- Python module: structured logging, Prometheus metrics, health checks
- JavaScript module: browser observability, performance tracking
- Complete documentation: architecture, Kubernetes, external services

**Service Instrumentation**
- Nexus: refactored to use shared library (~120 lines code reduction)
- Schema Service: added metrics, health probes, logging
- Prediction Workers (3×): model training/inference metrics, health checks
- Frontend: error tracking, query performance, custom metrics

**Documentation**
- API reference for all modules
- Integration guides for new services
- Kubernetes deployment patterns
- External service monitoring (Postgres, Trino, Redis)

**Architecture**
- Emit-only pattern (no forced observability backend)
- Service-specific metrics for ML pipelines
- Request ID and trace ID context propagation
- Kubernetes-native health probes (/healthz, /readyz)

All services now expose:
- JSON structured logs to stdout
- Prometheus metrics on /metrics endpoint
- Kubernetes health check endpoints
- Audit logging for compliance

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…servability library access

- Change prediction-worker build context from subdirectory to project root
- Update Dockerfiles to COPY shared/observability/python from root
- Set PYTHONPATH env var to include shared_obs directory
- Update observability_init.py imports to use PYTHONPATH injection
- Enables prediction workers to access shared observability library

This fixes the Docker build issue where nested prediction worker services
couldn't access the parent shared/observability/ directory due to Docker's
build context isolation.

Deployment:
- Local: docker-compose -f docker/docker-compose.yml build
- Kubernetes: Shared library can be embedded in image or injected via ConfigMap
- Registry: Images tagged as actyze/prediction-worker-{type}:v{version}
…gres + Trino)

Covers observability packaging and shipping for complete Actyze platform:

**Actyze Services (6):**
- Nexus, Schema Service, 3× Prediction Workers, Frontend
- Native Prometheus metrics + JSON structured logging
- Built-in health probes (/healthz, /readyz)

**External Services (2):**
- PostgreSQL: postgres-exporter + query logging
- Trino: JMX metrics + query tracking via exporter

**Deployment Strategies:**
- Docker Compose: All 8 services + observability stack
- Kubernetes: StatefulSets, Deployments, ConfigMaps, Sidecars
- Production: Image tagging, registry push, Helm integration

**Monitoring Stack:**
- Prometheus scraping all 8 services
- Grafana dashboards (Actyze, PostgreSQL, Trino)
- Complete prometheus.yml configuration

**Complete Coverage:**
- 100% service observability
- Metrics: Prometheus format
- Logs: Structured JSON + query logs
- Health checks: Liveness + readiness probes
…stack

Testing levels covered:
- Level 1: Unit tests (module imports, Python/TypeScript compile)
- Level 2: Integration tests (services start, health probes respond)
- Level 3: E2E tests (metrics flow, logs are JSON, queries work)
- Level 4: Load tests (concurrent queries, resource monitoring)

Test coverage includes:
- All 6 Actyze services (Nexus, Schema, 3 Workers, Frontend)
- Both external services (PostgreSQL exporter, Trino exporter)
- Prometheus scraping and query performance
- Grafana data sources
- Docker Compose validation
- Kubernetes validation

Includes:
- 50+ test commands with expected outputs
- Automated test script (test-observability.sh)
- Validation checklist
- Test report template
- Quick copy-paste command reference

Ready for: local development, CI/CD pipelines, production validation
Provides fast testing entry points:
- One-command full test suite (60 seconds)
- 10 individual test cases (copy & paste)
- Test matrix for all 8 services
- Troubleshooting guide
- Full test report script
- CI/CD integration example

Quick reference:
- Health endpoints (all services)
- Metrics endpoints (all services)
- Prometheus scraping validation
- PostgreSQL exporter check
- Structured log verification
- Load testing

Expected outcomes documented for each test
…cript

Uses the existing ./docker/start.sh and ./docker/stop.sh scripts:
- ./start.sh — Start all services with health checks
- ./stop.sh — Stop services
- ./stop.sh --clean — Full cleanup

Profiles supported:
- local (default): Local PostgreSQL + Local Trino
- external: External services only
- postgres-only: Local DB + External Trino
- trino-only: Local Trino + External DB

Testing procedures:
- Quick tests (health, metrics, logs, Prometheus)
- Full validation script (8 tests in 1 minute)
- Service port reference (all 11 ports)
- Log/metrics viewing commands
- Troubleshooting guide

Ready to test all 8 services immediately
Step-by-step instructions:
1. Start Docker Desktop
2. Run ./start.sh from docker/ folder
3. Verify services with docker ps
4. Test observability endpoints

Includes:
- Docker daemon startup verification
- Service startup (30-60 seconds)
- Health check commands
- Troubleshooting for common issues
- Timeline expectations
- Visual checklist

Ready to go from zero to running services
Wires the shared observability library into all Python services and
fixes three bugs surfaced by the aggregated health check (all services
now report healthy):

- Rename shared logging.py -> structured_logging.py to stop shadowing
  Python's stdlib logging (circular-import crash on startup).
- Route schema-service through its local observability_init importlib
  loader instead of `from shared.observability import ...`, which failed
  to resolve in-container.
- base.py: replace removed httpx._utils.default_timer() with stdlib
  time.perf_counter() (schema-service health check was crashing).
- memory_cache_service: return "connected": True so an empty in-process
  cache is not reported unhealthy.
- llm_service: health_check() checks the initialized client instead of
  non-existent attributes.
- Dockerfiles + docker-compose build contexts copy the shared library
  into each image.

Docs: refresh README, docker/README, OBSERVABILITY_COMPLETE with the
emit-only scope and verified-healthy status.
@roman1887 roman1887 added the area/observability Logging, metrics, tracing label Jul 13, 2026
Comment thread nexus/main.py
logger.error("readiness_check_failed", error=str(e))
return Response(
status_code=503,
content=f'{{"status": "not_ready", "message": "{str(e)}"}}',
Comment thread nexus/main.py
Comment on lines +248 to +252
return {
"status": "unhealthy",
"service": "nexus",
"error": str(e)
}
Comment thread nexus/main.py
logger.error("predictions_health_check_failed", error=str(e))
return Response(
status_code=503,
content=f'{{"status": "unhealthy", "error": "{str(e)}"}}',
Comment thread nexus/app/audit_logger.py
"""Audit logging for compliance and tracking."""

import structlog
import json
Comment thread nexus/app/audit_logger.py

import structlog
import json
from typing import Any, Dict, Optional
Comment thread nexus/app/audit_logger.py
import structlog
import json
from typing import Any, Dict, Optional
from datetime import datetime
Comment thread nexus/app/logging.py
import sys
from typing import Any, Dict
import structlog
from typing import Optional
Comment thread nexus/main.py

from app.config import settings
from app.logging import configure_logging
from app.logging import configure_logging, set_request_id, get_request_id
import sys
import time
from pathlib import Path
from typing import Optional
Comment on lines +20 to +27
from app.observability_init import (
setup_observability,
setup_health_endpoints,
get_logger,
MetricsContext,
record_sql_execution,
set_service_health,
)
@github-actions

Copy link
Copy Markdown

Claude Code Review

Code Review: fix(observability): shared library integration + health-check fixes

Overall Assessment

The PR achieves its stated goals and fixes real bugs. Most issues are moderate — the code is functional but has several correctness problems, a mild security concern, and significant maintainability debt from the importlib approach.


1. Security

[MEDIUM] SQL query text logged verbatim in audit logger

nexus/app/audit_logger.py lines 51 and 75 log query_text and sql_query directly:

self.logger.info("nl_query_executed", ..., query_text=query_text, generated_sql=generated_sql, ...)
self.logger.info("sql_execution", ..., sql_query=sql_query, ...)

Structured logs are commonly shipped to external log aggregators (Datadog, Splunk, etc.). Full SQL with embedded string literals can contain PII or secrets. The JS metrics layer has a sanitizeQuery() scrubber — no equivalent exists on the backend. Recommend either truncating/hashing the SQL in the audit log or documenting that log destinations must be treated as sensitive.

[LOW] Error message echoed into JSON response string via f-string

In nexus/main.py:

content=f'{{"status": "not_ready", "message": "{str(e)}"}}'

If the exception message contains a double-quote or backslash, this produces malformed JSON. Worse, it could expose internal exception details (file paths, connection strings) to the caller. Use json.dumps({"status": "not_ready", "message": str(e)}) instead. This pattern appears in both /readyz and /api/health/predictions.

[INFO] Auth bypass surface unchanged — The verify_secret pattern in prediction workers remains identical to before; the refactor doesn't regress it. No new issues introduced.


2. Correctness

[HIGH] Dead health_check method placed after uvicorn.run()

schema-service/schema_service.py ends with:

if __name__ == "__main__":
    uvicorn.run(app, host="0.0.0.0", port=int(os.getenv("PORT", "8001")))

    async def health_check(self) -> dict:  # ← UNREACHABLE

This method is syntactically valid Python but will never be executed and never attached to any class. It is dead code and the intent is unclear — it appears to be a leftover fragment that should have been placed on SchemaService before the if __name__ block.

[HIGH] Schema service /health endpoint loses diagnostic data

The old /health returned last_updated, total_schemas, and index_size from the embedder. The new setup_health_endpoints() returns only {"status": "healthy", "service": ..., "timestamp": ...}. This information is useful for the aggregated /health in Nexus. The schema-service-specific state is silently dropped.

[MEDIUM] observability_init.py files export configure_observability() but main.py imports configure_logging/get_logger/setup_health_endpoints

All three prediction worker observability_init.py files define configure_observability() but the actual imports in main.py are:

from observability_init import (
    configure_logging,
    get_logger,
    setup_health_endpoints,
)

None of those three names are defined in observability_init.py. This will raise ImportError on startup. The workers rely on sys.path.insert making the shared library directly importable, but configure_logging, get_logger, and setup_health_endpoints would then need to come from the shared library modules — and setup_health_endpoints is not part of the shared library per the schema-service implementation. This needs a concrete resolution: either add the missing exports to observability_init.py or change the imports in main.py.

[MEDIUM] MetricsContext timing is double-counted

In every /train and /predict handler:

start_time = time.time()
try:
    with MetricsContext("POST", "/train") as ctx:
        result = handler.train(...)
        duration = time.time() - start_time  # measures from before MetricsContext
        record_prediction_duration("xgboost", duration)

MetricsContext presumably records its own HTTP duration internally. record_prediction_duration is then called with a second, overlapping measurement from the same start_time. The two will diverge if MetricsContext.__enter__ does non-trivial work. Use time.time() consistently relative to the context entry, or drop the outer start_time and let MetricsContext own the measurement.

[MEDIUM] Error-path duration in prediction workers uses time.time() after MetricsContext raises

    except ValueError as e:
        duration = time.time() - start_time
        record_prediction("xgboost", status="error")
        record_prediction_duration("xgboost", duration)

MetricsContext as a context manager will call __exit__ on exception before reaching the except block. Whether it re-raises or suppresses the exception determines whether the except block runs at all. If MetricsContext.__exit__ suppresses and swallows the exception (even transiently), the error metrics would never be recorded. This depends on the shared library implementation that isn't in the diff, but it's worth verifying.

[LOW] useFirstContentfulPaint hook misuses ResizeObserver for FCP measurement

frontend/src/hooks/usePerformanceTracking.ts:

const resizeObserver = new ResizeObserver(() => {
  trackPerformance(`fcp_${featureName}`, performance.now(), 'ms', ...)
  resizeObserver.disconnect();
});

performance.now() here gives the time since page load, not the duration until first content — it is a monotonic clock reading, not a delta. The value will be in the thousands of milliseconds from page origin. The intent appears to be performance.now() - pageStartTime or using the PerformanceObserver API with paint entries. This produces incorrect FCP data.

[LOW] nexus/app/logging.py reads settings.LOG_LEVEL with wrong attribute name

log_level=getattr(settings, "LOG_LEVEL", "INFO"),

The existing settings object (FastAPI/pydantic-settings) would conventionally use lowercase log_level matching the field name in Settings. The old code referenced settings.log_level. If LOG_LEVEL doesn't exist on the model, getattr silently falls back to "INFO", ignoring any configured log level.


3. Code Quality

[MEDIUM] Significant duplication across three observability_init.py files

docker/prediction-worker-xgboost/observability_init.py, docker/prediction-worker-lightgbm/observability_init.py, and docker/prediction-worker-autogluon/observability_init.py are character-for-character identical except for the default service_name. At the same time there is schema-service/observability_init.py and schema-service/app/observability_init.py doing the same thing. The importlib loader pattern is reasonable for the in-container path problem, but five copies of it is unmaintainable. A single shared/observability/python/loader.py helper that other modules import would eliminate this.

[LOW] import json unused in audit_logger.py

import json

Imported at the top of nexus/app/audit_logger.py but never referenced. Remove it.

[LOW] console.warn in production frontend code

frontend/src/hooks/usePerformanceTracking.ts:

console.warn(`[${operationName}] endTracking called without startTracking`);

Project standards prohibit unnecessary console.log/debug statements. console.warn in a hook that ships to production violates this. Route through the observability logger instead.

[LOW] from observability.metrics import ... in prediction workers with no observability package

The main.py files for each prediction worker do:

from observability.metrics import MetricsContext, record_prediction, record_prediction_duration

There is no observability/ package installed or copied into those images. This would fail with ModuleNotFoundError unless sys.path.insert in observability_init.py (when it runs) already makes shared/observability/python a package root with a sub-module named metrics. This is fragile — it only works if observability_init is imported before this line and the path manipulation has taken effect. Module-level import ordering in Python makes this order-dependent.

[INFO] from app.logging import get_request_id imported inside function body

In nexus/app/api.py, imports like from app.logging import get_request_id appear inside the function body of generate_sql. This is a pattern used to avoid circular imports but should be documented or resolved at the module level if possible.


4. Testing

No tests accompany this PR. The PR adds:

  • A new AuditLogger class with 8 public methods
  • New middleware in nexus/main.py
  • New health/readiness endpoints
  • Three new React hooks
  • A new ErrorBoundary component
  • Observability init code across 5+ services

None of these have tests in the diff. The project standards require pytest coverage for backend and Jest for frontend. Given the number of correctness bugs found above (dead health_check method, ImportError in prediction workers, double-counted metrics), even basic unit tests would have caught these before review. This is the most significant gap in the PR.


5. AGPL Compliance

[LOW] Several new Python files are missing the AGPL-3.0 license header

Files without headers:

  • nexus/app/audit_logger.py
  • nexus/app/metrics.py
  • nexus/app/logging.py (modified, not new — acceptable)
  • schema-service/app/observability_init.py
  • schema-service/observability_init.py
  • All three docker/prediction-worker-*/observability_init.py

The frontend files (ErrorBoundary.tsx, hooks, observability-init.ts) correctly include SPDX-License-Identifier: AGPL-3.0-only. The Python files should follow suit.

[INFO] No new dependencies are introduced that conflict with AGPL-3.0. prometheus_client is Apache-2.0 compatible.


Summary of Blockers Before Merge

Severity Issue
High Dead health_check method after uvicorn.run() in schema_service.py — clarify intent
High configure_logging, get_logger, setup_health_endpoints not exported from prediction worker observability_init.py — will ImportError at startup
Medium f-string JSON construction in /readyz and /api/health/predictions can produce malformed JSON and leaks exception internals
Medium Schema service /health drops all diagnostic state
Medium Double-counted timing in prediction worker metrics handlers
Low settings.LOG_LEVEL vs settings.log_level typo silently degrades log level config
Low No tests for any new code

Automated review by Claude Sonnet 4.6 | Context: CLAUDE.md + README.md

Comment on lines +20 to +35
import {
trackQuery as trackQueryCore,
trackPerformance as trackPerformanceCore,
trackApiCall as trackApiCallCore,
trackComponentRender as trackComponentRenderCore,
trackMemoryUsage as trackMemoryUsageCore,
getQueryMetrics,
getPerformanceMetrics,
getMetricsByName,
getMetricStatistics,
clearMetrics,
exportMetrics,
type QueryMetrics,
type PerformanceMetric,
type Metric,
} from '../../shared/observability/javascript';
The nexus, schema-service, and prediction-worker Dockerfiles now COPY
shared/observability/python, so they must build from the repo root, not
their own subdirectory. Fix both image workflows to match:

- build-and-push-images.yml: nexus and schema-service context -> repo
  root; prediction-worker matrix uses an explicit dockerfile path with
  root context.
- release-build.yml: add the 3 prediction workers to the build matrix
  (were missing); correct the stale frontend path
  (docker/Dockerfile.frontend -> frontend/Dockerfile) and schema-service
  path (models/schema-service -> schema-service); build Python services
  and workers from repo root; list all 6 images in the release notes.

Without this, a v0.1.0 release build fails and pushes to main break
after the observability Dockerfile changes land.
@github-actions github-actions Bot added the ci/cd label Jul 13, 2026
@github-actions

Copy link
Copy Markdown

Claude Code Review

Code Review: fix(observability): shared library integration + health-check fixes

Overall Assessment

This is a substantial PR integrating a shared observability library across multiple services. The core fixes are correct and the approach is sound, but there are several issues worth addressing before merging.


1. Security

No hardcoded secrets detected. The workflow changes correctly use ${{ secrets.NAME }} throughout.

Issue: Error message injection in /readyz and /api/health/predictionsnexus/main.py

# Line ~240
content=f'{{"status": "not_ready", "message": "{str(e)}"}}',
# Line ~295
content=f'{{"status": "unhealthy", "error": "{str(e)}"}}',

Interpolating exception messages directly into raw JSON strings is both a JSON injection risk (if the exception message contains " or }) and a potential information disclosure issue. Use json.dumps or return a dict and let FastAPI serialize it.

# Replace with:
return Response(
    status_code=503,
    content=json.dumps({"status": "not_ready", "message": str(e)}),
    media_type="application/json"
)

Issue: query_text and sql_query logged in full in audit_logger.py

log_nl_query and log_sql_execution emit the full query text at INFO level. In a structured-log pipeline this is intentional for audit, but there is no truncation or redaction guard. If a user embeds a credential in a query (common mistake), it will appear in log aggregators in plain text. This is pre-existing behavior in spirit, but now it's being codified into a named audit system — worth flagging for a follow-up. At minimum add a note in the docstring.


2. Correctness

Bug: Dead code at module level in schema_service.py

if __name__ == "__main__":
    uvicorn.run(...)

    async def health_check(self) -> dict:   # ← unreachable
        ...

The health_check method is defined inside the if __name__ == "__main__" block, making it dead code. It was presumably meant to be a method on SchemaService. This will never be called.

Bug: observability_init.py exposes configure_logging/get_logger/setup_health_endpoints but the module doesn't define them

In docker/prediction-worker-autogluon/main.py:

from observability_init import (
    configure_logging,
    get_logger,
    setup_health_endpoints,
)

But docker/prediction-worker-autogluon/observability_init.py only defines configure_observability (and imports modules as obs_*). configure_logging, get_logger, and setup_health_endpoints are never explicitly exported — this will raise ImportError at startup. Same issue in xgboost and lightgbm workers.

Bug: MetricsContext used in prediction workers but never imported into observability_init.py's namespace for re-export

The prediction worker main.py files do:

from observability.metrics import MetricsContext, record_prediction, record_prediction_duration

observability.metrics doesn't exist as a package-level import path. After sys.path.insert(0, shared_obs_path), the module would be metrics not observability.metrics. This will fail at runtime.

Issue: useFirstContentfulPaint misuses performance.now() as an FCP timestamp

// frontend/src/hooks/usePerformanceTracking.ts
trackPerformance(`fcp_${featureName}`, performance.now(), 'ms', ...)

performance.now() returns milliseconds since navigation start, not a duration. This records wall-clock offset rather than a meaningful "time to paint" — the tracked value is meaningless as an FCP metric. Should be performance.now() - pageLoadStart or use the PerformanceObserver API.

Issue: console.warn left in production code

// frontend/src/hooks/usePerformanceTracking.ts line ~106
console.warn(`[${operationName}] endTracking called without startTracking`);

Project standards explicitly prohibit console.log/debug statements. console.warn in a production hook violates this. Replace with the structured logger.

Issue: Timing inconsistency in prediction workers

start_time = time.time() is set before the MetricsContext, but MetricsContext likely also tracks its own timing. The duration computed with time.time() - start_time outside the context duplicates what MetricsContext should be doing. This is a minor redundancy but could result in double-counting in dashboards.

Issue: /health deprecated but not removed from workers

The health endpoints on prediction workers are removed and replaced by setup_health_endpoints(app, ...) — but the schema service's setup_health_endpoints registers /health as a simple liveness probe that returns no service-specific information (last_updated, total_schemas, index_size). The original health endpoint was richer. This is a regression for operators who relied on those fields.


3. Code Quality

importlib.util pattern is copied identically across 5 files — three prediction workers, schema-service/observability_init.py, and schema-service/app/observability_init.py. Two of these (schema-service/observability_init.py vs schema-service/app/observability_init.py) appear to do the same thing with different relative paths. The one at schema-service/observability_init.py is likely unused since schema_service.py imports from app.observability_init. If so, delete it.

import json is missing in nexus/app/audit_logger.py

import json  # ← imported but never used

Actually it's imported and never used — remove it.

settings.log_level vs settings.LOG_LEVEL inconsistency in nexus/app/logging.py

log_level=getattr(settings, "LOG_LEVEL", "INFO"),

Using getattr with a fallback suggests uncertainty about the attribute name. Elsewhere in the codebase settings.log_level (lowercase) is used. This should be consistent.

isObservabilityReady re-exported but isReady wrapper is redundant

frontend/src/utils/observability-init.ts wraps a single-line re-export with a new function name. Minor, but creates an unnecessary indirection.

nexus/app/api.py imports inside function bodies

async def generate_sql(...):
    import time
    from app.metrics import record_nl_query
    from app.audit_logger import audit_logger
    from app.logging import get_request_id

Function-level imports are an anti-pattern and inconsistent with the rest of the codebase. These should be module-level imports.


4. Testing

No tests are included for any of the new observability code:

  • The AuditLogger class has no test coverage
  • The new middleware in nexus/main.py is untested
  • The ErrorBoundary component has no __tests__/ directory entry
  • The new hooks (usePerformanceTracking, useQueryObservability) have no Jest tests

For infrastructure this central (audit logging touches every SQL execution path), the absence of tests is a meaningful gap. At minimum, a test that AuditLogger.log_sql_execution emits structured output with the expected keys, and a test that the ErrorBoundary renders the fallback UI on error, would be expected per project standards.


5. AGPL Compliance

Backend Python files (audit_logger.py, metrics.py, logging.py) are missing AGPL-3.0 license headers. The frontend files have them (SPDX-License-Identifier: AGPL-3.0-only). This is inconsistent — the Python files should follow the same convention.


Summary Table

Category Severity Item
Security Medium Raw f-string JSON in error responses (injection + disclosure)
Security Low Unbounded query text in audit logs
Correctness High health_check is dead code in schema_service.py
Correctness High ImportError at startup for all 3 prediction workers
Correctness High from observability.metrics import wrong path in workers
Correctness Medium useFirstContentfulPaint records wrong value
Correctness Low console.warn in production hook
Quality Medium Function-level imports in api.py
Quality Low Duplicate observability_init.py in schema-service
Quality Low Unused import json in audit_logger.py
Testing Medium No tests for audit logger, middleware, ErrorBoundary, hooks
AGPL Low Missing license headers in Python backend files

The two High correctness bugs (dead health_check method and ImportError in prediction workers) will cause startup failures and should be fixed before merge.


Automated review by Claude Sonnet 4.6 | Context: CLAUDE.md + README.md

Worker Dockerfiles COPY shared/observability/python and
docker/prediction-worker-*/, both repo-root-relative, but the compose
build context was `.` (the docker/ dir), so those COPYs could not
resolve. Set context to `..` to match nexus and schema-service.
@github-actions

Copy link
Copy Markdown

Claude Code Review

Code Review: fix(observability): shared library integration + health-check fixes

Overall Assessment

Solid observability integration with several genuine issues to address. No secrets found in the diff.


1. Security

Auth bypass on health/metrics endpoints (Medium)

setup_health_endpoints() in schema-service/app/observability_init.py registers /health, /ready, and /metrics without any auth. The /metrics endpoint could expose internal topology and cardinality data to unauthenticated callers. The existing verify_service_auth dependency is not applied. Same concern applies to /metrics in nexus/main.py. Consider whether these should be restricted to internal network only, or at minimum document the intentional exposure.

Error message leakage in readiness probe (Low)

# nexus/main.py:~248
content=f'{{"status": "not_ready", "message": "{str(e)}"}}',

Unescaped exception messages injected into JSON via f-string. If the exception message contains ", this produces malformed JSON and could leak internal details. Use json.dumps instead:

import json
content=json.dumps({"status": "not_ready", "message": str(e)})

Same pattern appears in /api/health/predictions.

sys.path.insert(0, ...) security surface (Low)

All three observability_init.py files unconditionally insert the shared path at position 0 of sys.path on every import. If the container ever gets a path traversal to write into that directory, it achieves code execution. This is an accepted trade-off for the importlib pattern but worth noting. At minimum, assert the path is what you expect:

assert shared_obs_path.is_dir(), f"Shared observability path not found: {shared_obs_path}"

2. Correctness

Dead code after uvicorn.run() in schema_service.py (Critical)

# schema-service/schema_service.py
if __name__ == "__main__":
    uvicorn.run(app, host="0.0.0.0", port=int(os.getenv("PORT", "8001")))

    async def health_check(self) -> dict:   # ← UNREACHABLE

This health_check method definition was pasted after uvicorn.run() inside the if __name__ block — it is unreachable dead code and doesn't belong to any class. It silently does nothing. This needs to be moved to the appropriate class or removed.

observability_init.py files export configure_observability() but callers import configure_logging() / setup_health_endpoints() (High)

The three prediction-worker observability_init.py files only define configure_observability(). But main.py for each worker imports configure_logging, get_logger, and setup_health_endpoints from observability_init:

from observability_init import (
    configure_logging,
    get_logger,
    setup_health_endpoints,
)

None of these names are defined in any of those observability_init.py files — configure_observability() is defined, but the three imported names don't exist. This will raise ImportError at startup. The schema-service/app/observability_init.py does export these, but the worker files don't re-export them from obs_structured_logging etc. This is a startup crash.

obs_metrics.get_metrics_registry() may not exist (Medium)

# schema-service/app/observability_init.py
metrics_registry = obs_metrics.get_metrics_registry()

This function is called inside the /metrics FastAPI route but we don't see it defined anywhere in the diff. If metrics.py doesn't export get_metrics_registry, this will 500 on every metrics scrape.

MetricsContext used with both start_time = time.time() and internal timing (Medium)

In all three worker main.py files, start_time = time.time() is set before entering with MetricsContext(...), and then duration = time.time() - start_time is computed inside the context. If MetricsContext also tracks duration internally (typical for context managers used this way), duration is being double-counted — once by the explicit start_time and once internally. Verify and unify.

from observability.metrics import ... in worker main.py files (High)

from observability.metrics import (
    MetricsContext,
    record_prediction,
    record_prediction_duration,
)

observability here would need to be a package importable as observability.metrics. After sys.path.insert(0, shared_obs_path), the path points to shared/observability/python/, so import metrics would work but import observability.metrics would not (there's no observability/ subdirectory inside python/). This will also raise ImportError at startup.

configure_logging called before get_logger result is meaningful (Low)

In worker main.py files, configure_logging(...) is called at module load time, but the logger is obtained immediately after. If configure_logging is idempotent and structlog is already partially configured from the shared library import side effects, the ordering may work — but it's fragile. The shared library should be designed so get_logger before configure_logging returns a safe default.

useFirstContentfulPaint metric is wrong (Low)

// frontend/src/hooks/usePerformanceTracking.ts
trackPerformance(`fcp_${featureName}`, performance.now(), 'ms', ...)

This records performance.now() (time since page load) as the FCP value, not the time since the component mounted. The hook is measuring "time since page load when the first resize was observed", not "time to first content paint for this feature". The intent is unclear but the current implementation is misleading.

useOperationTracking has a console.warn (project standards violation)

console.warn(`[${operationName}] endTracking called without startTracking`);

Project standards prohibit console.log/console.warn debug statements. Use the structured logger.


3. Code Quality

Massive importlib duplication — no shared installer function (Medium)

The exact same 15-line importlib boilerplate (load spec, create module, exec module, repeat ×3) is copy-pasted verbatim into five files. This should be a utility function in one place, e.g. shared/observability/python/loader.py:

def load_obs_module(name, filename): ...

nexus/app/logging.py settings attribute name inconsistency (Low)

log_level=getattr(settings, "LOG_LEVEL", "INFO"),

The rest of the codebase uses settings.log_level (lowercase, as Pydantic BaseSettings fields are lowercase by convention). getattr(settings, "LOG_LEVEL", "INFO") will always return "INFO" if the field is actually log_level. Check which is correct.

nexus/app/api.py — lazy imports inside request handler (Low)

async def generate_sql(...):
    import time
    from app.metrics import record_nl_query
    from app.audit_logger import audit_logger
    from app.logging import get_request_id

Module-level imports are the project convention and avoids repeated import overhead on every request. Move these to the top of the file.

import json in audit_logger.py is unused

import json  # ← never used

schema-service/observability_init.py vs schema-service/app/observability_init.py — two files, overlapping purpose (Medium)

There's a schema-service/observability_init.py (root-level) and a schema-service/app/observability_init.py. The root one is never imported by anything in the diff — schema_service.py imports from app.observability_init. The root file is dead code that adds confusion. Remove it or document its purpose.

frontend/src/utils/observability-init.ts imports from ../../shared/observability/javascript

This is a relative path that traverses above src/. Whether this resolves correctly depends on the TypeScript/webpack config. If frontend/ is the TypeScript root, ../../shared goes outside the project root and will likely fail at build time unless paths aliases are configured. This needs verification and a tsconfig.json paths entry or a proper package reference.

release-build.yml frontend build context changed from . to ./frontend

The previous workflow had BUILD_CONTEXT=. for frontend. If frontend/Dockerfile uses ADD . . from the repo root, this change will break the frontend build. Verify the frontend Dockerfile's COPY/ADD instructions are compatible with ./frontend as build context.


4. Testing

No tests are added for any of the new modules:

  • nexus/app/audit_logger.py — no tests
  • nexus/app/metrics.py — no tests
  • The middleware in nexus/main.py — no tests
  • New frontend hooks — no tests

The observability_init.py startup-crash bugs (#2 above) would have been caught by even minimal smoke tests. Given the PR claims "all services healthy", it's likely the prediction workers were not tested with this exact code (the import paths are broken as written).

At minimum: a pytest fixture that imports each worker's main.py module would catch the ImportError at startup.


5. AGPL Compliance

✅ New Python files lack AGPL headers but the project standard only firmly requires them for "new source files" — the existing Python files in the codebase appear to also lack headers, so this is consistent.
✅ New frontend files (ErrorBoundary.tsx, hooks, observability-init.ts) include SPDX-License-Identifier: AGPL-3.0-only headers — correct.
✅ No new dependencies identified in the diff that conflict with AGPL-3.0. prometheus_client is Apache-2.0, compatible.


Summary of Blockers

# Severity Issue
1 Critical Dead health_check method after uvicorn.run() in schema_service.py
2 Critical Prediction worker main.py imports configure_logging, get_logger, setup_health_endpoints from observability_init — these names don't exist there → startup crash
3 High from observability.metrics import ... in worker main.py — wrong import path → startup crash
4 Medium Unescaped exception strings in JSON responses in readiness/health endpoints
5 Medium get_metrics_registry() called but likely not defined in shared metrics module

Items 2 and 3 mean the prediction workers cannot start. The PR description says "all services healthy" — either these workers weren't tested, or there's code in the shared library not shown in the diff that makes it work. Either way, this needs confirmation before merge.


Automated review by Claude Sonnet 4.6 | Context: CLAUDE.md + README.md

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