feat(observability): Add embedding cache metrics - #11969
Conversation
|
👋 Hi h-avsha! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughEmbedding-cache Prometheus metric names now use a shared prefix, optional cache metric families are registered during vLLM setup, model labels reach the connector, and scheduler cache operations emit validated metrics. ChangesEmbedding Cache Metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
components/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py (2)
232-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the repeated
EmbeddingCacheMetricsimport to the top of the file.
from dynamo.common.utils.prometheus import EmbeddingCacheMetrics as ECMis re-imported inside 4 separate test methods (lines 233, 244, 264, 281). This isn't an optional/collection-resilience import (noimportorskip), so it should live at module scope once.🔧 Proposed fix
+from dynamo.common.utils.prometheus import EmbeddingCacheMetrics as ECM + class TestSchedulerMetrics: ... def test_series_present_before_activity(self): - from dynamo.common.utils.prometheus import EmbeddingCacheMetrics as ECM - conn = self._make_connector()(repeat removal for the other 3 methods)
As per coding guidelines: "Keep imports at the top of the file; always flag
importstatements inside function bodies, methods, or classes as they hide dependencies and make modules harder to understand."Also applies to: 243-244, 263-264, 280-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py` around lines 232 - 241, Move the EmbeddingCacheMetrics import aliased as ECM to module scope at the top of the test file, then remove the repeated local imports from all four affected test methods, including test_series_present_before_activity. Keep the existing metric references unchanged.Source: Coding guidelines
210-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
LABELSasClassVar(Ruff RUF012).
LABELS = {"model": "test-model", "dynamo_component": "backend"}is a mutable dict assigned directly as a class attribute; static analysis flags this.🔧 Proposed fix
+from typing import ClassVar ... - LABELS = {"model": "test-model", "dynamo_component": "backend"} + LABELS: ClassVar[dict[str, str]] = {"model": "test-model", "dynamo_component": "backend"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py` at line 210, Annotate the mutable class attribute LABELS as a ClassVar in the relevant test class, preserving its existing dictionary value and keys so Ruff RUF012 no longer flags it.Source: Linters/SAST tools
components/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.py (2)
76-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeferred
prometheus_clientimport inside__init__.Per coding guidelines, imports inside function/method bodies should be flagged as they hide dependencies. Here it's a deliberate, documented pattern (must import
prometheus_clientonly afterPROMETHEUS_MULTIPROC_DIRis inherited from the parent process), mirroring the same technique already used inregister_embedding_cache_metricsincomponents/src/dynamo/common/utils/prometheus.py. Flagging for visibility per the guideline, but no action needed given the established precedent and technical constraint.As per coding guidelines: "Keep imports at the top of the file; always flag
importstatements inside function bodies, methods, or classes as they hide dependencies and make modules harder to understand."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.py` around lines 76 - 90, No code change is needed: retain the deferred prometheus_client import in __init__ because it must occur after PROMETHEUS_MULTIPROC_DIR is inherited, consistent with register_embedding_cache_metrics.Source: Coding guidelines
54-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMetric definitions duplicated between this class and
register_embedding_cache_metrics.
_SchedulerCacheMetricsandregister_embedding_cache_metrics(incomponents/src/dynamo/common/utils/prometheus.py) both define the same six metrics with the same help text, through two different registration mechanisms. If one path adds a metric or edits help text, the other can silently drift. Consider extracting the(EmbeddingCacheMetrics member, help text)pairs into one shared source (e.g., a small dict or list of tuples inEmbeddingCacheMetrics/prometheus.py) that both call sites iterate over.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.py` around lines 54 - 137, Centralize the six metric definitions used by _SchedulerCacheMetrics and register_embedding_cache_metrics into one shared source in the EmbeddingCacheMetrics/prometheus module, including each metric member and help text. Update both registration paths to iterate over that shared definition while preserving their existing label handling, counter/gauge types, and multiprocess behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/vllm/main.py`:
- Around line 218-231: Replace the duplicated
"DynamoMultimodalEmbeddingCacheConnector" literals with a shared connector-name
constant defined in cache_config.py, and import and reuse that constant in the
engine_metric_prefixes selection in main.py. Ensure ec_connector comparisons and
assignments continue using the exact same value.
---
Nitpick comments:
In
`@components/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.py`:
- Around line 76-90: No code change is needed: retain the deferred
prometheus_client import in __init__ because it must occur after
PROMETHEUS_MULTIPROC_DIR is inherited, consistent with
register_embedding_cache_metrics.
- Around line 54-137: Centralize the six metric definitions used by
_SchedulerCacheMetrics and register_embedding_cache_metrics into one shared
source in the EmbeddingCacheMetrics/prometheus module, including each metric
member and help text. Update both registration paths to iterate over that shared
definition while preserving their existing label handling, counter/gauge types,
and multiprocess behavior.
In
`@components/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py`:
- Around line 232-241: Move the EmbeddingCacheMetrics import aliased as ECM to
module scope at the top of the test file, then remove the repeated local imports
from all four affected test methods, including
test_series_present_before_activity. Keep the existing metric references
unchanged.
- Line 210: Annotate the mutable class attribute LABELS as a ClassVar in the
relevant test class, preserving its existing dictionary value and keys so Ruff
RUF012 no longer flags it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 298c96da-eba7-41e5-a700-b6c02cbbc3f0
📒 Files selected for processing (5)
components/src/dynamo/common/utils/prometheus.pycomponents/src/dynamo/vllm/main.pycomponents/src/dynamo/vllm/multimodal_utils/cache_config.pycomponents/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.pycomponents/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py
|
Hi @h-avsha, thanks for the contribution! Can you retroactively sign your previous commit and any future commits to pass the DCO check: https://github.com/ai-dynamo/dynamo/pull/11969/checks?check_run_id=88740627995? |
|
/ok to test 920354a |
This comment has been minimized.
This comment has been minimized.
|
By the way, the failing "PR / deploy-operator (push)" test checks are a known issue unrelated to this PR that will be resolved in US timezone tomorrow - one of our automation tokens expired there |
|
/ok to test f36cad6 |
|
/ok to test f36cad6 |
|
/ok to test b88589c |
|
/ok to test 796f71b |
|
/ok to test 796f71b |
|
/ok to test f645756 |
1 similar comment
|
/ok to test f645756 |
|
/ok to test 851e859 |
1 similar comment
|
/ok to test 851e859 |
Signed-off-by: h-avsha <avshalom.manevich@hcompany.ai>
In data-parallel deployments each EngineCore has its own scheduler-side CPU cache; without a per-rank label the mostrecent gauges collapse to a single process's snapshot while counters keep summing. Add a dp_rank label (from parallel_config.data_parallel_rank), mirroring the kvstats gauges in LLMBackendMetrics, so gauges partition per rank and mostrecent only dedups dead-pid vs live-replacement within a rank. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: h-avsha <avshalom.manevich@hcompany.ai>
Signed-off-by: h-avsha <avshalom.manevich@hcompany.ai>
Signed-off-by: h-avsha <avshalom.manevich@hcompany.ai>
|
/ok to test c10532c |
Overview:
Add Prometheus metrics to
DynamoMultimodalEmbeddingCacheConnector, the vLLM EC connector implementing the CPU tier of the multimodal embedding cache. Cache effectiveness (hit rate, occupancy, eviction pressure) was previously invisible in production.Details:
EmbeddingCacheMetricsfamily (dynamo_component_embedding_cache_{hits,misses,evictions}_total,_utilization,_current_bytes,_entries) — the same namesregister_embedding_cache_metricsuses for the worker-layerMultimodalEmbeddingCacheManager, so dashboards see one metric family regardless of which cache implementation serves the model. Worker-role instances emit nothing (they do no cache accounting)./metrics. Transport rides on the existing multiprocess Prometheus setup:PROMETHEUS_MULTIPROC_DIRis set before engine start, soprometheus_clientmmap-persists values that the frontend'sMultiProcessCollectorpicks up. Metrics live in a privateCollectorRegistry(never double-exported through the globalREGISTRY); gauges usemultiprocess_mode="mostrecent"so dead-pid values don't pollute aggregation.setup_metrics_collectionforwards the family from the multiproc registry only when this connector is configured — encode-routing deployments expose the same names in-process viaregister_embedding_cache_metrics, so unconditional forwarding would double-expose.configure_multimodal_embedding_cachegains amodel_nameparameter and passesmodel_name/componentthroughec_connector_extra_config— the scheduler-side connector has no other channel to learn its Dynamo identity for label values.update_state_after_alloc: hits on the load path, misses on the save path (oversized items count as misses), evictions per LRU pop, gauges after each insert. Note a repeat image only reaches this connector after a GPUEncoderCacheManagermiss — GPU-cache and prefix-cache hits bypass the CPU tier by design.ec_bothdeployment (122B multimodal model): 26 unique images → 26 misses/entries with byte-exactcurrent_bytes/utilization, and a confirmed CPU-tier hit after overflowing the GPU encoder-cache budget. Also works under plainvllm serve(no Dynamo frontend): vLLM's/metricscollects the multiproc dir unfiltered, so the family appears there with no extra wiring.Where should the reviewer start?
components/src/dynamo/vllm/multimodal_utils/multimodal_embedding_cache_connector.py—_SchedulerCacheMetricsand the accounting call sites; the class docstring explains the cross-process transport.components/src/dynamo/vllm/main.py— conditionalengine_metric_prefixesinsetup_metrics_collection(the double-exposure guard).components/src/dynamo/vllm/multimodal_utils/cache_config.py— label-value plumbing viaec_connector_extra_config.components/src/dynamo/common/utils/prometheus.py— just extractsEMBEDDING_CACHE_METRIC_PREFIXfrom the existing enum.components/src/dynamo/vllm/tests/multimodal_utils/test_vllm_multimodal_embedding_cache_connector.py—TestSchedulerMetrics: worker-role silence, zero-valued series at init, hit/miss/usage accounting, evictions, oversized items.Related Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes