From b779627fa649b278d290d888286ab074177b338c Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 3 Jun 2026 02:01:28 +0200 Subject: [PATCH] fix(observability): address third review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remaining items from the PR #831 Claude review: - processor span symmetry: add "vector_sync.total_chars" to the sparse embedding span (already on the dense span) and drop the redundant "embedding.batch_size" attribute from both spans — it always equalled vector_sync.chunk_count and would mislead once batching is split. - metrics: document the deliberate "throughput counts only on full success" contract in record_document_parse (partial extractions flagged success=False are counted as a parse-error but never inflate pages/chars/bytes throughput). - config: extract _detect_base_provider() -> (family, model) as the single source of truth for the provider-detection priority chain, shared by get_embedding_model_name() and get_embedding_provider_family(). Preserves the intentional gateway asymmetry (only the family method short-circuits). - base.py: Optional[...] -> PEP 604 `... | None`; drop now-unused import. Behavior unchanged (get_embedding_* outputs covered by test_config.py). Refs Deck #175, PR #831. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/config.py | 84 +++++++++---------- .../document_processors/base.py | 13 ++- nextcloud_mcp_server/observability/metrics.py | 4 + nextcloud_mcp_server/vector/processor.py | 3 +- 4 files changed, 49 insertions(+), 55 deletions(-) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 6da74196..92445d06 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -917,16 +917,45 @@ class Settings: self.enable_multi_user_basic_auth = resolved_mode == "multi_user_basic" self.enable_login_flow = resolved_mode == "login_flow" - def get_embedding_model_name(self) -> str: + def _detect_base_provider(self) -> tuple[str, str]: """ - Get the active embedding model name based on provider priority. + Resolve the ``(family, model)`` for the underlying embedding provider. - Priority order (same as ProviderRegistry): + Single source of truth for the provider-detection priority chain shared + by ``get_embedding_model_name`` and ``get_embedding_provider_family``: 1. Bedrock - if AWS_REGION or BEDROCK_EMBEDDING_MODEL is set 2. OpenAI - if OPENAI_API_KEY is set 3. Mistral - if MISTRAL_API_KEY is set 4. Ollama - if OLLAMA_BASE_URL is set - 5. Simple - fallback (returns "simple-{dimension}") + 5. Simple - fallback + + Does NOT handle the gateway short-circuit — callers layer that on top + as needed (see the asymmetry note on ``get_embedding_model_name``). + """ + if ( + self.aws_region + or self.bedrock_embedding_model + or self.bedrock_generation_model + ): + return "bedrock", self.bedrock_embedding_model or "bedrock-default" + + if self.openai_api_key: + return "openai", self.openai_embedding_model + + if self.mistral_api_key: + return "mistral", self.mistral_embedding_model + + if self.ollama_base_url: + return "ollama", self.ollama_embedding_model + + return "simple", f"simple-{self.simple_embedding_dimension}" + + def get_embedding_model_name(self) -> str: + """ + Get the active embedding model name based on provider priority. + + Priority order (same as ProviderRegistry): bedrock → openai → mistral → + ollama → simple (returns "simple-{dimension}"). Returns: Active embedding model name @@ -937,23 +966,7 @@ class Settings: # get_embedding_provider_family() short-circuits to the gateway-routed # family. Keep that asymmetry in mind before joining metrics/labels # derived from these two methods. - if ( - self.aws_region - or self.bedrock_embedding_model - or self.bedrock_generation_model - ): - return self.bedrock_embedding_model or "bedrock-default" - - if self.openai_api_key: - return self.openai_embedding_model - - if self.mistral_api_key: - return self.mistral_embedding_model - - if self.ollama_base_url: - return self.ollama_embedding_model - - return f"simple-{self.simple_embedding_dimension}" + return self._detect_base_provider()[1] def get_embedding_provider_family(self) -> str: """ @@ -964,14 +977,9 @@ class Settings: *family* (e.g. "bedrock"), never the model name, to keep metric cardinality bounded. - Priority mirrors ``get_embedding_model_name`` / ProviderRegistry: - 1. Gateway - if EMBEDDING_PROVIDER=gateway (family from the model prefix, - e.g. "mistral/mistral-embed" -> "mistral") - 2. Bedrock - if AWS_REGION or BEDROCK_EMBEDDING_MODEL is set - 3. OpenAI - if OPENAI_API_KEY is set - 4. Mistral - if MISTRAL_API_KEY is set - 5. Ollama - if OLLAMA_BASE_URL is set - 6. Simple - fallback + Gateway short-circuits to the gateway-routed family (from the model + prefix, e.g. "mistral/mistral-embed" -> "mistral"); otherwise the family + comes from the shared ``_detect_base_provider`` priority chain. Returns: Provider family: gateway-routed family | bedrock | openai | mistral @@ -981,23 +989,7 @@ class Settings: model = self.embedding_gateway_model or "" return model.split("/", 1)[0] if "/" in model else "gateway" - if ( - self.aws_region - or self.bedrock_embedding_model - or self.bedrock_generation_model - ): - return "bedrock" - - if self.openai_api_key: - return "openai" - - if self.mistral_api_key: - return "mistral" - - if self.ollama_base_url: - return "ollama" - - return "simple" + return self._detect_base_provider()[0] def get_collection_name(self) -> str: """ diff --git a/nextcloud_mcp_server/document_processors/base.py b/nextcloud_mcp_server/document_processors/base.py index 86ab1850..cb36ded2 100644 --- a/nextcloud_mcp_server/document_processors/base.py +++ b/nextcloud_mcp_server/document_processors/base.py @@ -2,7 +2,7 @@ from abc import ABC, abstractmethod from collections.abc import Awaitable, Callable -from typing import Any, Optional +from typing import Any from pydantic import BaseModel @@ -22,7 +22,7 @@ class ProcessingResult(BaseModel): success: bool = True """Whether processing succeeded""" - error: Optional[str] = None + error: str | None = None """Error message if processing failed""" @@ -83,11 +83,10 @@ class DocumentProcessor(ABC): self, content: bytes, content_type: str, - filename: Optional[str] = None, - options: Optional[dict[str, Any]] = None, - progress_callback: Optional[ - Callable[[float, Optional[float], Optional[str]], Awaitable[None]] - ] = None, + filename: str | None = None, + options: dict[str, Any] | None = None, + progress_callback: Callable[[float, float | None, str | None], Awaitable[None]] + | None = None, ) -> ProcessingResult: """Process a document and extract text. diff --git a/nextcloud_mcp_server/observability/metrics.py b/nextcloud_mcp_server/observability/metrics.py index 5cc00e6c..bcf7e373 100644 --- a/nextcloud_mcp_server/observability/metrics.py +++ b/nextcloud_mcp_server/observability/metrics.py @@ -533,6 +533,10 @@ def record_document_parse( processor=processor, tier=tier, status=status ).observe(duration) document_parse_total.labels(processor=processor, tier=tier, status=status).inc() + # Throughput counters (pages/chars/bytes) accrue only on a full success. + # A partial extraction flagged success=False is recorded above as a + # parse-error but is intentionally excluded here so low-confidence output + # never inflates pipeline throughput. if status == "success": if pages > 0: document_pages_processed_total.labels(processor=processor, tier=tier).inc( diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 658b785e..cfd17567 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -631,7 +631,6 @@ async def _index_document( "embedding.kind": "dense", "embedding.provider": provider, "embedding.model": settings.get_embedding_model_name(), - "embedding.batch_size": len(chunk_texts), }, ): embedding_service = get_embedding_service() @@ -659,9 +658,9 @@ async def _index_document( "vector_sync.embed_sparse", attributes={ _ATTR_CHUNK_COUNT: len(chunk_texts), + "vector_sync.total_chars": total_chars, "embedding.kind": "sparse", "embedding.provider": "bm25", - "embedding.batch_size": len(chunk_texts), }, ): bm25_service = await get_bm25_service()