From d314ae204b52309975f95163042618eef09f41ee Mon Sep 17 00:00:00 2001 From: adityamparikh Date: Fri, 24 Apr 2026 14:13:26 -0400 Subject: [PATCH 1/3] feat: add SLF4J logging to all service classes Add SLF4J loggers to CollectionService, IndexingService, SchemaService, SearchService, and JsonUtils. Log exceptions in all catch blocks instead of silently swallowing them. Use appropriate log levels: error for operational failures, warn for recoverable issues, debug for expected conditions (Solr 10 metrics unavailability, individual doc failures). Safe for STDIO mode: logback-spring.xml already suppresses console logging in the stdio profile. Closes #1 Signed-off-by: Aditya Parikh Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: adityamparikh --- .../server/collection/CollectionService.java | 21 ++++++++++++------- .../mcp/server/indexing/IndexingService.java | 15 +++++++++++-- .../solr/mcp/server/schema/SchemaService.java | 5 +++++ .../solr/mcp/server/search/SearchService.java | 4 ++++ .../solr/mcp/server/util/JsonUtils.java | 5 +++++ 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java index 011d278e..a94b3819 100644 --- a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java +++ b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java @@ -45,6 +45,8 @@ import org.apache.solr.mcp.server.config.SolrConfigurationProperties; import org.apache.solr.mcp.server.util.PromptNames; import org.jspecify.annotations.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springaicommunity.mcp.annotation.McpArg; import org.springaicommunity.mcp.annotation.McpComplete; import org.springaicommunity.mcp.annotation.McpPrompt; @@ -136,6 +138,8 @@ @Observed public class CollectionService { + private static final Logger logger = LoggerFactory.getLogger(CollectionService.class); + // ======================================== // Constants for API Parameters and Paths // ======================================== @@ -683,16 +687,17 @@ public QueryStats buildQueryStats(QueryResponse response) { * Internal cache metrics fetch that assumes the collection has already been * validated and the name has been extracted from any shard identifier. */ - private @Nullable CacheStats fetchCacheMetrics(String collection) { + private @Nullable CacheStats fetchCacheMetrics(String collectionName) { try { - NamedList coreMetrics = fetchMetrics(collection, CACHE_METRIC_PREFIX); + NamedList coreMetrics = fetchMetrics(collectionName, CACHE_METRIC_PREFIX); if (coreMetrics == null) { return null; } CacheStats stats = extractCacheStats(coreMetrics); return isCacheStatsEmpty(stats) ? null : stats; - } catch (SolrServerException | IOException | RuntimeException _) { + } catch (SolrServerException | IOException | RuntimeException e) { + logger.debug("Cache metrics unavailable for collection: {}", collectionName, e); return null; } } @@ -799,18 +804,19 @@ private CacheStats extractCacheStats(NamedList coreMetrics) { * Internal handler metrics fetch that assumes the collection has already been * validated and the name has been extracted from any shard identifier. */ - private @Nullable HandlerStats fetchHandlerMetrics(String collection) { + private @Nullable HandlerStats fetchHandlerMetrics(String collectionName) { try { // Handler metrics are flat keys (e.g. QUERY./select.requests) so we // fetch each handler prefix separately and reconstruct HandlerInfo - HandlerInfo selectHandler = fetchFlatHandlerInfo(collection, SELECT_HANDLER_METRIC_PREFIX, + HandlerInfo selectHandler = fetchFlatHandlerInfo(collectionName, SELECT_HANDLER_METRIC_PREFIX, SELECT_HANDLER_KEY); - HandlerInfo updateHandler = fetchFlatHandlerInfo(collection, UPDATE_HANDLER_METRIC_PREFIX, + HandlerInfo updateHandler = fetchFlatHandlerInfo(collectionName, UPDATE_HANDLER_METRIC_PREFIX, UPDATE_HANDLER_KEY); HandlerStats stats = new HandlerStats(selectHandler, updateHandler); return isHandlerStatsEmpty(stats) ? null : stats; - } catch (SolrServerException | IOException | RuntimeException _) { + } catch (SolrServerException | IOException | RuntimeException e) { + logger.debug("Handler metrics unavailable for collection: {}", collectionName, e); return null; } } @@ -1080,6 +1086,7 @@ public SolrHealthStatus checkHealth(@McpToolParam(description = "Solr collection statsResponse.getResults().getNumFound(), Instant.now(), actualCollection); } catch (Exception e) { + logger.warn("Health check failed for collection: {}", collection, e); return new SolrHealthStatus(false, e.getMessage(), null, null, Instant.now(), actualCollection); } } diff --git a/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java b/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java index 5ac3704d..13504967 100644 --- a/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java +++ b/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java @@ -29,6 +29,8 @@ import org.apache.solr.mcp.server.indexing.documentcreator.IndexingDocumentCreator; import org.apache.solr.mcp.server.util.PromptNames; import org.apache.solr.mcp.server.util.PromptText; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springaicommunity.mcp.annotation.McpArg; import org.springaicommunity.mcp.annotation.McpPrompt; import org.springaicommunity.mcp.annotation.McpTool; @@ -116,6 +118,8 @@ @Observed public class IndexingService { + private static final Logger logger = LoggerFactory.getLogger(IndexingService.class); + private static final int DEFAULT_BATCH_SIZE = 1000; /** SolrJ client for communicating with Solr server */ @@ -578,12 +582,14 @@ public int indexDocuments(String collection, List documents) solrClient.add(collection, batch); successCount += batch.size(); } catch (SolrServerException | IOException | RuntimeException e) { + logger.warn("Batch indexing failed, retrying individually", e); // Try indexing documents individually to identify problematic ones for (SolrInputDocument doc : batch) { try { solrClient.add(collection, doc); successCount++; - } catch (SolrServerException | IOException | RuntimeException _) { + } catch (SolrServerException | IOException | RuntimeException e2) { + logger.debug("Failed to index individual document", e2); // Document failed to index - this is expected behavior for problematic // documents // We continue processing the rest of the batch @@ -592,7 +598,12 @@ public int indexDocuments(String collection, List documents) } } - solrClient.commit(collection); + try { + solrClient.commit(collection); + } catch (SolrServerException | IOException e) { + logger.error("Failed to commit after indexing to collection: {}", collection, e); + throw e; + } return successCount; } diff --git a/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java b/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java index 3f3bb96a..73bb3405 100644 --- a/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java +++ b/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java @@ -33,6 +33,8 @@ import org.apache.solr.client.solrj.request.schema.SchemaRequest; import org.apache.solr.client.solrj.response.schema.SchemaRepresentation; import org.apache.solr.mcp.server.util.PromptNames; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springaicommunity.mcp.annotation.McpArg; import org.springaicommunity.mcp.annotation.McpPrompt; import org.springaicommunity.mcp.annotation.McpResource; @@ -137,6 +139,8 @@ @Observed public class SchemaService { + private static final Logger logger = LoggerFactory.getLogger(SchemaService.class); + /** SolrJ client for communicating with Solr server */ private final SolrClient solrClient; @@ -185,6 +189,7 @@ public String getSchemaResource(String collection) { try { return toJson(objectMapper, getSchema(collection)); } catch (Exception e) { + logger.error("Failed to get schema for collection: {}", collection, e); // Serialise via Jackson rather than concatenating: an exception message // containing a quote, backslash or newline would otherwise emit invalid // JSON to the MCP client. diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java index 2fb9800c..782126e7 100644 --- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java +++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java @@ -34,6 +34,8 @@ import org.apache.solr.common.params.FacetParams; import org.apache.solr.mcp.server.util.PromptNames; import org.jspecify.annotations.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springaicommunity.mcp.annotation.McpArg; import org.springaicommunity.mcp.annotation.McpPrompt; import org.springaicommunity.mcp.annotation.McpTool; @@ -108,6 +110,8 @@ @Observed public class SearchService { + private static final Logger logger = LoggerFactory.getLogger(SearchService.class); + /** * Fragments of Solr's own error text that identify a failure we can advise on. * diff --git a/src/main/java/org/apache/solr/mcp/server/util/JsonUtils.java b/src/main/java/org/apache/solr/mcp/server/util/JsonUtils.java index 6ecc3bc1..44c36a8d 100644 --- a/src/main/java/org/apache/solr/mcp/server/util/JsonUtils.java +++ b/src/main/java/org/apache/solr/mcp/server/util/JsonUtils.java @@ -18,6 +18,8 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Utility class for JSON serialization operations. @@ -31,6 +33,8 @@ */ public final class JsonUtils { + private static final Logger logger = LoggerFactory.getLogger(JsonUtils.class); + private JsonUtils() { // Utility class - prevent instantiation } @@ -52,6 +56,7 @@ public static String toJson(ObjectMapper objectMapper, Object obj) { try { return objectMapper.writeValueAsString(obj); } catch (JsonProcessingException e) { + logger.error("Failed to serialize response", e); return "{\"error\": \"Failed to serialize response\"}"; } } From 8fd803ad4a4379f454cdf518859f13773f21e638 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Thu, 20 Aug 2026 11:04:23 -0400 Subject: [PATCH 2/3] refactor(collection): narrow the metrics catch clauses to SolrException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts the narrowing from #111 so the two PRs compose instead of colliding. Both PRs rewrite the same two catch clauses in fetchCacheMetrics and fetchHandlerMetrics. #111 narrows RuntimeException to SolrException; this PR was binding the exception for logging while leaving RuntimeException in place. Whichever merged second would either conflict or silently revert the other's intent — so this branch now carries the narrowed form too, and the end state is the same in either merge order. RemoteSolrException extends SolrException (verified against solrj 10.0.0), so the Solr 10 path where /admin/mbeans is gone still degrades to null rather than propagating. What no longer gets swallowed is unrelated RuntimeExceptions -- which is the point of #111, and is what the new debug logging is there to surface. Signed-off-by: Aditya Parikh --- .../apache/solr/mcp/server/collection/CollectionService.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java index a94b3819..67860822 100644 --- a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java +++ b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java @@ -40,6 +40,7 @@ import org.apache.solr.client.solrj.response.LukeResponse; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.client.solrj.response.SolrPingResponse; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.NamedList; import org.apache.solr.mcp.server.config.SolrConfigurationProperties; @@ -696,7 +697,7 @@ public QueryStats buildQueryStats(QueryResponse response) { CacheStats stats = extractCacheStats(coreMetrics); return isCacheStatsEmpty(stats) ? null : stats; - } catch (SolrServerException | IOException | RuntimeException e) { + } catch (SolrServerException | IOException | SolrException e) { logger.debug("Cache metrics unavailable for collection: {}", collectionName, e); return null; } @@ -815,7 +816,7 @@ private CacheStats extractCacheStats(NamedList coreMetrics) { HandlerStats stats = new HandlerStats(selectHandler, updateHandler); return isHandlerStatsEmpty(stats) ? null : stats; - } catch (SolrServerException | IOException | RuntimeException e) { + } catch (SolrServerException | IOException | SolrException e) { logger.debug("Handler metrics unavailable for collection: {}", collectionName, e); return null; } From 6319e19c09b747908016d29b91b37b298f702b1b Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Fri, 11 Sep 2026 10:18:47 -0400 Subject: [PATCH 3/3] refactor(search): log query failures before rethrowing with a hint SearchService gained a logger in the SLF4J commit but never used it: #166 landed afterwards and made every SolrException rethrow wrapped in an IllegalArgumentException carrying a remediation hint, so nothing is swallowed there any more. The MCP client sees only the exception message, so the server kept no record of a failed query at all. Log one debug line in withRemediationHint covering every path, including the no-hint fallback. Debug rather than warn because the failure is already reported to the caller; the stdio profile defines no appenders, so this cannot reach stdout. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015rXANeQAujgxhEz1uAWFm9 Signed-off-by: Aditya Parikh --- .../java/org/apache/solr/mcp/server/search/SearchService.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java index 782126e7..c9dda135 100644 --- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java +++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java @@ -374,6 +374,10 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que private static RuntimeException withRemediationHint(SolrException e, String collection) { final String message = String.valueOf(e.getMessage()); + // The MCP client only ever sees the exception message, so without this the + // server keeps no record of a failed query. + logger.debug("Solr query failed on collection {}", collection, e); + // An unknown collection is a 404 whose body is Solr's HTML "not found" page, // so SolrJ reports it as a mime-type mismatch and leaves getMetadata() null. // The status code is the only signal that survives; match it rather than the