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.
This commit is contained in:
parent
443c94c827
commit
cf4bd598b4
4 changed files with 35 additions and 4 deletions
|
|
@ -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=[
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue