From cf4bd598b42398245bee3956940e90d20279af3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 7 Apr 2026 09:42:59 +0200 Subject: [PATCH] fix: make bank_id metric label opt-in to prevent OTel memory leak (#898) * fix: make bank_id metric label opt-in to prevent OTel memory leak bank_id as an OTel metric attribute creates unbounded histogram growth since each unique bank_id produces never-evicted time series. Default to excluding it; opt in with HINDSIGHT_API_METRICS_INCLUDE_BANK_ID=true for deployments with few banks. Closes #850 * refactor: use config.py for metrics_include_bank_id setting Move HINDSIGHT_API_METRICS_INCLUDE_BANK_ID from direct os.getenv in metrics.py to the standard HindsightConfig path. Add configuration documentation. --- hindsight-api-slim/hindsight_api/config.py | 5 ++++ hindsight-api-slim/hindsight_api/metrics.py | 6 ++++- hindsight-api-slim/tests/test_metrics.py | 27 ++++++++++++++++--- .../docs/developer/configuration.md | 1 + 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/hindsight-api-slim/hindsight_api/config.py b/hindsight-api-slim/hindsight_api/config.py index d1ccfbaa..e0cd185f 100644 --- a/hindsight-api-slim/hindsight_api/config.py +++ b/hindsight-api-slim/hindsight_api/config.py @@ -269,6 +269,7 @@ ENV_OTEL_EXPORTER_OTLP_ENDPOINT = "HINDSIGHT_API_OTEL_EXPORTER_OTLP_ENDPOINT" ENV_OTEL_EXPORTER_OTLP_HEADERS = "HINDSIGHT_API_OTEL_EXPORTER_OTLP_HEADERS" ENV_OTEL_SERVICE_NAME = "HINDSIGHT_API_OTEL_SERVICE_NAME" ENV_OTEL_DEPLOYMENT_ENVIRONMENT = "HINDSIGHT_API_OTEL_DEPLOYMENT_ENVIRONMENT" +ENV_METRICS_INCLUDE_BANK_ID = "HINDSIGHT_API_METRICS_INCLUDE_BANK_ID" # Vertex AI configuration ENV_LLM_VERTEXAI_PROJECT_ID = "HINDSIGHT_API_LLM_VERTEXAI_PROJECT_ID" @@ -552,6 +553,7 @@ DEFAULT_DISPOSITION_EMPATHY = None DEFAULT_OTEL_TRACES_ENABLED = False # Disabled by default for backward compatibility DEFAULT_OTEL_SERVICE_NAME = "hindsight-api" DEFAULT_OTEL_DEPLOYMENT_ENVIRONMENT = "development" +DEFAULT_METRICS_INCLUDE_BANK_ID = False # Disabled by default to avoid high-cardinality OTel metric growth # Audit log defaults DEFAULT_AUDIT_LOG_ENABLED = False # Disabled by default @@ -877,6 +879,7 @@ class HindsightConfig: otel_exporter_otlp_headers: str | None otel_service_name: str otel_deployment_environment: str + metrics_include_bank_id: bool # Audit log configuration (static - server-level only) audit_log_enabled: bool # Master switch for audit logging @@ -1419,6 +1422,8 @@ class HindsightConfig: otel_exporter_otlp_headers=os.getenv(ENV_OTEL_EXPORTER_OTLP_HEADERS) or None, otel_service_name=os.getenv(ENV_OTEL_SERVICE_NAME, DEFAULT_OTEL_SERVICE_NAME), otel_deployment_environment=os.getenv(ENV_OTEL_DEPLOYMENT_ENVIRONMENT, DEFAULT_OTEL_DEPLOYMENT_ENVIRONMENT), + metrics_include_bank_id=os.getenv(ENV_METRICS_INCLUDE_BANK_ID, str(DEFAULT_METRICS_INCLUDE_BANK_ID)).lower() + in ("true", "1", "yes"), # Audit log configuration (static, server-level only) audit_log_enabled=os.getenv(ENV_AUDIT_LOG_ENABLED, str(DEFAULT_AUDIT_LOG_ENABLED)).lower() == "true", audit_log_actions=[ diff --git a/hindsight-api-slim/hindsight_api/metrics.py b/hindsight-api-slim/hindsight_api/metrics.py index 1e29b7ac..166571c9 100644 --- a/hindsight-api-slim/hindsight_api/metrics.py +++ b/hindsight-api-slim/hindsight_api/metrics.py @@ -252,6 +252,9 @@ class MetricsCollector(MetricsCollectorBase): def __init__(self): self.meter = get_meter() + from .config import get_config + + self._include_bank_id = get_config().metrics_include_bank_id # Operation latency histogram (in seconds) # Records duration of retain, recall, reflect operations @@ -332,10 +335,11 @@ class MetricsCollector(MetricsCollectorBase): start_time = time.time() attributes = { "operation": operation, - "bank_id": bank_id, "source": source, "tenant": _get_tenant(), } + if self._include_bank_id: + attributes["bank_id"] = bank_id if budget: attributes["budget"] = budget if max_tokens: diff --git a/hindsight-api-slim/tests/test_metrics.py b/hindsight-api-slim/tests/test_metrics.py index 5db167c4..81824f95 100644 --- a/hindsight-api-slim/tests/test_metrics.py +++ b/hindsight-api-slim/tests/test_metrics.py @@ -76,7 +76,10 @@ class TestMetricsCollector: @pytest.fixture def collector(self, mock_meter): """Create a MetricsCollector with a mock meter.""" - with patch("hindsight_api.metrics.get_meter", return_value=mock_meter): + mock_config = MagicMock() + mock_config.metrics_include_bank_id = False + with patch("hindsight_api.metrics.get_meter", return_value=mock_meter), \ + patch("hindsight_api.config.get_config", return_value=mock_config): return MetricsCollector() def test_record_operation_records_duration(self, collector): @@ -95,7 +98,7 @@ class TestMetricsCollector: # Second arg is attributes dict attributes = call_args[0][1] assert attributes["operation"] == "recall" - assert attributes["bank_id"] == "test_bank" + assert "bank_id" not in attributes # excluded by default to avoid high-cardinality OTel growth assert attributes["source"] == "api" assert attributes["success"] == "true" @@ -166,6 +169,21 @@ class TestMetricsCollector: assert reflect_attrs["operation"] == "reflect" assert reflect_attrs["source"] == "api" + def test_record_operation_includes_bank_id_when_enabled(self): + """Test that bank_id is included in attributes when metrics_include_bank_id is enabled.""" + mock_config = MagicMock() + mock_config.metrics_include_bank_id = True + with patch("hindsight_api.metrics.get_meter") as mock_get_meter, \ + patch("hindsight_api.config.get_config", return_value=mock_config): + mock_get_meter.return_value = MagicMock() + collector = MetricsCollector() + + with collector.record_operation("recall", bank_id="test_bank", source="api"): + pass + + attributes = collector.operation_duration.record.call_args[0][1] + assert attributes["bank_id"] == "test_bank" + class TestGetMetricsCollector: """Tests for the get_metrics_collector function.""" @@ -269,7 +287,10 @@ class TestLLMMetrics: @pytest.fixture def collector(self, mock_meter): """Create a MetricsCollector with a mock meter.""" - with patch("hindsight_api.metrics.get_meter", return_value=mock_meter): + mock_config = MagicMock() + mock_config.metrics_include_bank_id = False + with patch("hindsight_api.metrics.get_meter", return_value=mock_meter), \ + patch("hindsight_api.config.get_config", return_value=mock_config): return MetricsCollector() def test_record_llm_call_records_duration(self, collector): diff --git a/hindsight-docs/docs/developer/configuration.md b/hindsight-docs/docs/developer/configuration.md index 310d0769..acec8cf4 100644 --- a/hindsight-docs/docs/developer/configuration.md +++ b/hindsight-docs/docs/developer/configuration.md @@ -1100,6 +1100,7 @@ Hindsight provides OpenTelemetry-based observability for LLM calls, conforming t | `HINDSIGHT_API_OTEL_EXPORTER_OTLP_HEADERS` | Headers for OTLP exporter (format: "key1=value1,key2=value2") | - | | `HINDSIGHT_API_OTEL_SERVICE_NAME` | Service name for traces | `hindsight-api` | | `HINDSIGHT_API_OTEL_DEPLOYMENT_ENVIRONMENT` | Deployment environment name (e.g., development, staging, production) | `development` | +| `HINDSIGHT_API_METRICS_INCLUDE_BANK_ID` | Include `bank_id` in OTel metric attributes. Enable only for deployments with few banks — high cardinality causes unbounded memory growth. | `false` | **Features:** - Full prompts and completions recorded as events