From 2ba08e68c1e379d17b19f46f12927df18bb00fbc Mon Sep 17 00:00:00 2001 From: Yucheng Zhu Date: Wed, 19 Aug 2026 14:04:49 -0700 Subject: [PATCH] fix(observability): make the lock result contract one source of truth The cron-lock metric documented three result values while the code emitted a fourth, `error`, added when a failed attempt was split from losing the election. A consumer building alerts from the documented set would silently drop every Redis-outage attempt, which is the case the split existed to surface. Rather than adding the missing word, the four outcomes are now a `LockAttemptResult` enum that both sides derive from: the lock manager is typed to emit only its members, and the metric documentation is generated from them, so the two cannot drift again. The test parses the advertised list out of the documentation rather than substring-matching it, since `error` also appears in the prose that follows and would have made a looser assertion pass against the very documentation that prompted this. --- litellm/integrations/prometheus.py | 10 +++-- .../db_transaction_queue/pod_lock_manager.py | 9 +++-- litellm/types/integrations/prometheus.py | 12 ++++++ .../test_prometheus_cronjob_lock_metrics.py | 37 +++++++++++++++++++ 4 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 tests/test_litellm/integrations/test_prometheus_cronjob_lock_metrics.py diff --git a/litellm/integrations/prometheus.py b/litellm/integrations/prometheus.py index c7c9d758c76..2cbdf36cfdd 100644 --- a/litellm/integrations/prometheus.py +++ b/litellm/integrations/prometheus.py @@ -756,8 +756,10 @@ class PrometheusLogger(CustomLogger): self.litellm_cronjob_lock_acquisitions_total = self._counter_factory( name="litellm_cronjob_lock_acquisitions_total", documentation=( - "Attempts to take the single-owner lock for a cron job. result is one of acquired, " - "not_acquired, no_redis; no_redis means no Redis is configured, so no pod can be elected" + "Attempts to take the single-owner lock for a cron job. result is one of " + f"{', '.join(r.value for r in LockAttemptResult)}; no_redis means no Redis is " + "configured, so no pod can be elected, and error means the attempt itself failed " + "rather than losing the election" ), labelnames=("cronjob_id", "result"), ) @@ -3211,9 +3213,9 @@ class PrometheusLogger(CustomLogger): if run.items_processed: self.litellm_scheduled_job_items_processed_total.labels(job_name=run.job_name).inc(run.items_processed) - def record_cronjob_lock_attempt(self, cronjob_id: str, result: str) -> None: + def record_cronjob_lock_attempt(self, cronjob_id: str, result: LockAttemptResult) -> None: """Publish the outcome of one single-owner lock attempt.""" - self.litellm_cronjob_lock_acquisitions_total.labels(cronjob_id=cronjob_id, result=result).inc() + self.litellm_cronjob_lock_acquisitions_total.labels(cronjob_id=cronjob_id, result=result.value).inc() def record_db_pool_sample(self, update: DBPoolMetricsUpdate) -> None: """Publish one reading of this worker's Prisma connection pool.""" diff --git a/litellm/proxy/db/db_transaction_queue/pod_lock_manager.py b/litellm/proxy/db/db_transaction_queue/pod_lock_manager.py index 0627fa66c16..33b71627894 100644 --- a/litellm/proxy/db/db_transaction_queue/pod_lock_manager.py +++ b/litellm/proxy/db/db_transaction_queue/pod_lock_manager.py @@ -7,6 +7,7 @@ from litellm._uuid import uuid from litellm.caching.redis_cache import RedisCache from litellm.constants import DEFAULT_CRON_JOB_LOCK_TTL_SECONDS from litellm.proxy.db.db_transaction_queue.base_update_queue import service_logger_obj +from litellm.types.integrations.prometheus import LockAttemptResult from litellm.types.services import ServiceTypes if TYPE_CHECKING: @@ -15,7 +16,7 @@ else: ProxyLogging = Any -def _record_lock_attempt(cronjob_id: str, result: str) -> None: +def _record_lock_attempt(cronjob_id: str, result: LockAttemptResult) -> None: """Publish the outcome of one single-owner lock attempt. Each result is a distinct operational state: no Redis means no pod can ever @@ -70,15 +71,15 @@ end """ if self.redis_cache is None: verbose_proxy_logger.debug("redis_cache is None, skipping acquire_lock") - _record_lock_attempt(cronjob_id, "no_redis") + _record_lock_attempt(cronjob_id, LockAttemptResult.NO_REDIS) return None try: acquired: Final = await self._attempt_acquire_lock(cronjob_id, ttl=ttl, allow_reentrant=allow_reentrant) except Exception as e: verbose_proxy_logger.error("Error acquiring Redis lock for %s: %s", cronjob_id, e) - _record_lock_attempt(cronjob_id, "error") + _record_lock_attempt(cronjob_id, LockAttemptResult.ERROR) return False - _record_lock_attempt(cronjob_id, "acquired" if acquired else "not_acquired") + _record_lock_attempt(cronjob_id, LockAttemptResult.ACQUIRED if acquired else LockAttemptResult.NOT_ACQUIRED) return acquired async def _attempt_acquire_lock( diff --git a/litellm/types/integrations/prometheus.py b/litellm/types/integrations/prometheus.py index fe58af24662..9e745aa7a31 100644 --- a/litellm/types/integrations/prometheus.py +++ b/litellm/types/integrations/prometheus.py @@ -8,6 +8,18 @@ from typing import Any, ClassVar, Final, Literal import litellm +class LockAttemptResult(str, Enum): + """Closed set of outcomes for one cron-job lock attempt, so the `result` + label stays bounded and the metric documentation cannot drift from it.""" + + ACQUIRED = "acquired" + NOT_ACQUIRED = "not_acquired" + # No Redis is configured, so no pod can ever be elected. + NO_REDIS = "no_redis" + # The attempt itself failed, rather than losing the election. + ERROR = "error" + + def _sanitize_prometheus_label_name(label: str) -> str: """ Sanitize a label name to comply with Prometheus label name requirements. diff --git a/tests/test_litellm/integrations/test_prometheus_cronjob_lock_metrics.py b/tests/test_litellm/integrations/test_prometheus_cronjob_lock_metrics.py new file mode 100644 index 00000000000..ffc818b7d06 --- /dev/null +++ b/tests/test_litellm/integrations/test_prometheus_cronjob_lock_metrics.py @@ -0,0 +1,37 @@ +"""The cron-lock metric's advertised result values against the ones actually emitted. + +A consumer builds alerts from the documented set, so a value the code can emit +but the documentation omits is silently dropped from their queries. +""" + +import os +import re +import sys + +sys.path.insert(0, os.path.abspath("../../..")) + +import litellm +from litellm.integrations.prometheus import PrometheusLogger +from litellm.types.integrations.prometheus import LockAttemptResult + + +def test_the_lock_metric_documents_every_result_it_can_emit(monkeypatch): + monkeypatch.setattr(litellm, "callbacks", []) + monkeypatch.setattr(litellm, "success_callback", []) + from prometheus_client import REGISTRY + + for collector in list(REGISTRY._collector_to_names.keys()): + try: + REGISTRY.unregister(collector) + except Exception: + pass + + documentation = PrometheusLogger().litellm_cronjob_lock_acquisitions_total._documentation + + # The advertised set only, not the prose that follows it, so a value merely + # mentioned in passing does not count as documented. + advertised = re.search(r"result is one of ([^;]+);", documentation) + assert advertised is not None, f"no advertised result list in: {documentation}" + documented = {value.strip() for value in advertised.group(1).split(",")} + + assert documented == {result.value for result in LockAttemptResult}