-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: improve Code Index error messages with detailed diagnostics #7745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,15 +69,39 @@ export class GeminiEmbedder implements IEmbedder { | |
| async validateConfiguration(): Promise<{ valid: boolean; error?: string }> { | ||
| try { | ||
| // Delegate validation to the OpenAI-compatible embedder | ||
| // The error messages will be specific to Gemini since we're using Gemini's base URL | ||
| return await this.openAICompatibleEmbedder.validateConfiguration() | ||
| const result = await this.openAICompatibleEmbedder.validateConfiguration() | ||
|
|
||
| // If validation failed, enhance the error message with Gemini-specific guidance | ||
| if (!result.valid && result.error) { | ||
| // Check for common Gemini-specific issues | ||
| if ( | ||
| result.error.includes("401") || | ||
| result.error.includes("403") || | ||
| result.error.includes("Authentication") | ||
| ) { | ||
| result.error = `${result.error}. For Gemini, ensure you have a valid API key from Google AI Studio (makersuite.google.com/app/apikey) and that it's correctly configured.` | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the best approach for error enhancement? The string matching for "401", "403", "Authentication", "404", and "model" feels a bit fragile. Could we consider using a more structured approach with error codes or types instead of relying on string patterns? |
||
| } else if (result.error.includes("404") || result.error.includes("model")) { | ||
| result.error = `${result.error}. Supported Gemini models: text-embedding-004 (dimension: 768), gemini-embedding-001 (dimension: 2048).` | ||
| } else if (result.error.includes("connection") || result.error.includes("host")) { | ||
| result.error = `${result.error}. Gemini API endpoint: ${GeminiEmbedder.GEMINI_BASE_URL}` | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } catch (error) { | ||
| TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| location: "GeminiEmbedder:validateConfiguration", | ||
| modelId: this.modelId, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding more specific telemetry event types here. Instead of just capturing a generic CODE_INDEX_ERROR, we could track specific error types (auth_failure, connection_error, model_error) to get better insights into what's failing most often. |
||
| }) | ||
| throw error | ||
|
|
||
| // Provide a more informative error message | ||
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
| return { | ||
| valid: false, | ||
| error: `Gemini embedder validation failed: ${errorMessage}. Please check your API key and model configuration (current model: ${this.modelId}).`, | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,19 +93,50 @@ export class CodeIndexServiceFactory { | |
| */ | ||
| public async validateEmbedder(embedder: IEmbedder): Promise<{ valid: boolean; error?: string }> { | ||
| try { | ||
| return await embedder.validateConfiguration() | ||
| const result = await embedder.validateConfiguration() | ||
|
|
||
| // If validation failed but no specific error was provided, add context | ||
| if (!result.valid && !result.error) { | ||
| const config = this.configManager.getConfig() | ||
| result.error = | ||
| t("embeddings:validation.configurationError") + | ||
| ` (Provider: ${config.embedderProvider}, Model: ${config.modelId || "default"})` | ||
| } | ||
|
|
||
| return result | ||
| } catch (error) { | ||
| // Capture telemetry for the error | ||
| // Capture telemetry for the error with additional context | ||
| const config = this.configManager.getConfig() | ||
| TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| location: "validateEmbedder", | ||
| provider: config.embedderProvider, | ||
| model: config.modelId, | ||
| }) | ||
|
|
||
| // If validation throws an exception, preserve the original error message | ||
| // Provide detailed error message with context | ||
| let errorMessage = error instanceof Error ? error.message : String(error) | ||
|
|
||
| // Add provider-specific guidance | ||
| if (config.embedderProvider === "gemini") { | ||
| if (errorMessage.includes("401") || errorMessage.includes("403") || errorMessage.includes("API key")) { | ||
| errorMessage += | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this provider-specific guidance potentially duplicate what's already added in the embedder's own validateConfiguration method? For example, GeminiEmbedder already adds similar guidance for authentication errors. Maybe we should check if the error message already contains provider-specific guidance before adding more? |
||
| ". Please ensure your Gemini API key is valid and correctly configured in the settings." | ||
| } | ||
| } else if (config.embedderProvider === "openai") { | ||
| if (errorMessage.includes("401") || errorMessage.includes("403")) { | ||
| errorMessage += ". Please check your OpenAI API key in the settings." | ||
| } | ||
| } else if (config.embedderProvider === "ollama") { | ||
| if (errorMessage.includes("connection") || errorMessage.includes("ECONNREFUSED")) { | ||
| errorMessage += ". Please ensure Ollama is running locally and accessible." | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| valid: false, | ||
| error: error instanceof Error ? error.message : "embeddings:validation.configurationError", | ||
| error: errorMessage, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,34 +167,88 @@ export function handleValidationError( | |
| // Check for status-based errors first | ||
| const statusError = getErrorMessageForStatus(statusCode, embedderType) | ||
| if (statusError) { | ||
| return { valid: false, error: statusError } | ||
| // Include additional context for authentication errors | ||
| if (statusCode === 401 || statusCode === 403) { | ||
| const details = embedderType === "gemini" ? t("embeddings:validation.geminiAuthDetails") : "" | ||
| return { valid: false, error: details ? `${statusError} ${details}` : statusError } | ||
| } | ||
| // Include status code in error for better debugging | ||
| return { valid: false, error: `${statusError} (HTTP ${statusCode})` } | ||
| } | ||
|
|
||
| // Check for connection errors | ||
| // Check for connection errors with more specific messages | ||
| if (errorMessage) { | ||
| if ( | ||
| errorMessage.includes("ENOTFOUND") || | ||
| errorMessage.includes("ECONNREFUSED") || | ||
| errorMessage.includes("ETIMEDOUT") || | ||
| errorMessage === "AbortError" || | ||
| errorMessage.includes("HTTP 0:") || | ||
| errorMessage === "No response" | ||
| ) { | ||
| if (errorMessage.includes("ENOTFOUND")) { | ||
| const url = extractUrlFromError(errorMessage) | ||
| return { | ||
| valid: false, | ||
| error: t("embeddings:validation.hostNotFound", { url: url || "configured endpoint" }), | ||
| } | ||
| } | ||
|
|
||
| if (errorMessage.includes("ECONNREFUSED")) { | ||
| const url = extractUrlFromError(errorMessage) | ||
| return { | ||
| valid: false, | ||
| error: t("embeddings:validation.connectionRefused", { url: url || "configured endpoint" }), | ||
| } | ||
| } | ||
|
|
||
| if (errorMessage.includes("ETIMEDOUT")) { | ||
| return { | ||
| valid: false, | ||
| error: t("embeddings:validation.connectionTimeout"), | ||
| } | ||
| } | ||
|
|
||
| if (errorMessage === "AbortError" || errorMessage.includes("HTTP 0:") || errorMessage === "No response") { | ||
| return { valid: false, error: t("embeddings:validation.connectionFailed") } | ||
| } | ||
|
|
||
| if (errorMessage.includes("Failed to parse response JSON")) { | ||
| return { valid: false, error: t("embeddings:validation.invalidResponse") } | ||
| } | ||
|
|
||
| // Check for API key related errors | ||
| if (errorMessage.includes("API key") || errorMessage.includes("api key") || errorMessage.includes("api-key")) { | ||
| return { valid: false, error: t("embeddings:validation.invalidApiKey") } | ||
| } | ||
|
|
||
| // Check for model-related errors | ||
| if ( | ||
| errorMessage.includes("model") && | ||
| (errorMessage.includes("not found") || errorMessage.includes("does not exist")) | ||
| ) { | ||
| return { valid: false, error: t("embeddings:validation.modelNotAvailable") } | ||
| } | ||
| } | ||
|
|
||
| // For generic errors, preserve the original error message if it's not a standard one | ||
| if (errorMessage && errorMessage !== "Unknown error") { | ||
| return { valid: false, error: errorMessage } | ||
| // Provide more context with the error | ||
| return { | ||
| valid: false, | ||
| error: `${t("embeddings:validation.configurationError")}: ${errorMessage}`, | ||
| } | ||
| } | ||
|
|
||
| // Fallback to generic error | ||
| return { valid: false, error: t("embeddings:validation.configurationError") } | ||
| // Fallback to generic error with embedder type for context | ||
| return { | ||
| valid: false, | ||
| error: `${t("embeddings:validation.configurationError")} (${embedderType})`, | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Extracts URL from error message if present | ||
| */ | ||
| function extractUrlFromError(errorMessage: string): string | undefined { | ||
| // Try to extract URL from common error patterns | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex pattern here might miss some edge cases for URL extraction. Would it be worth using a more comprehensive URL pattern or even a dedicated URL parsing library for better reliability? |
||
| const urlMatch = errorMessage.match(/(?:https?:\/\/[^\s]+)|(?:getaddrinfo\s+\w+\s+([^\s]+))/i) | ||
| if (urlMatch) { | ||
| return urlMatch[1] || urlMatch[0] | ||
| } | ||
| return undefined | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add placeholder translations or TODO comments for other supported languages? I notice we only updated the English locale file, and it might be easy to forget updating the other language files later.