mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
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.
This commit is contained in:
parent
6586e455c3
commit
2ba08e68c1
4 changed files with 60 additions and 8 deletions
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
Loading…
Add table
Reference in a new issue