GIE-226: Add LLM-friendly errors and handle empty response cases

pkg/mcp/handlers.go

@@ -125,7 +125,14 @@ func ExecuteRangeQueryHandler(opts ObsMCPOptions) func(context.Context, mcp.Call
 		// Execute the range query
 		result, err := promClient.ExecuteRangeQuery(ctx, query, startTime, endTime, time.Duration(stepDuration))
 		if err != nil {
-			return errorResult(fmt.Sprintf("failed to execute range query: %s", err.Error()))
+			// Pass through the error directly as it's already LLM-friendly from the loader
+			return errorResult(err.Error())
+		}
+
+		// Check if query returned empty results - return helpful guidance as text
+		if emptyGuidance, ok := result["emptyResultGuidance"].(string); ok && emptyGuidance != "" {
+			slog.Info("ExecuteRangeQueryHandler returned empty results")
+			return mcp.NewToolResultText(emptyGuidance), nil
 		}
 
 		// Convert to structured output

pkg/prometheus/loader.go

@@ -3,10 +3,12 @@ package prometheus
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/prometheus/client_golang/api"
 	v1 "github.com/prometheus/client_golang/api/prometheus/v1"
+	"github.com/prometheus/common/model"
 )
 
 const (
@@ -51,6 +53,71 @@ func (p *RealLoader) WithGuardrails(g *Guardrails) *RealLoader {
 	return p
 }
 
+// makeLLMFriendlyError converts Prometheus errors into more descriptive, LLM-friendly messages
+func makeLLMFriendlyError(err error, query string) error {
+	if err == nil {
+		return nil
+	}
+
+	errMsg := err.Error()
+	lowerMsg := strings.ToLower(errMsg)
+
+	// Check for common error patterns and provide helpful context
+	switch {
+	case strings.Contains(lowerMsg, "parse error") || strings.Contains(lowerMsg, "bad_data"):
+		return fmt.Errorf("the PromQL query '%s' has a syntax error. Error details: %w. "+
+			"Please check the query syntax and ensure all metric names, labels, and functions are correctly formatted",
+			query, err)
+
+	case strings.Contains(lowerMsg, "unknown function"):
+		return fmt.Errorf("the PromQL query '%s' uses an unknown function. Error details: %w. "+
+			"Please verify that the function name is correct and supported by Prometheus",
+			query, err)
+
+	case strings.Contains(lowerMsg, "timeout") || strings.Contains(lowerMsg, "deadline exceeded"):
+		return fmt.Errorf("the query '%s' took too long to execute and timed out. Error details: %w. "+
+			"This might happen if the query is too complex, the time range is too large, or the Prometheus server is under heavy load. "+
+			"Try reducing the time range, increasing the step size, or simplifying the query",
+			query, err)
+
+	case strings.Contains(lowerMsg, "no such host") || strings.Contains(lowerMsg, "connection refused"):
+		return fmt.Errorf("cannot connect to the Prometheus server. Error details: %w. "+
+			"Please verify that the Prometheus server is running and accessible", err)
+
+	default:
+		// Return error with query context for better debugging
+		return fmt.Errorf("query '%s' failed: %w", query, err)
+	}
+}
+
+// checkEmptyResult provides helpful context when a query returns no data
+func checkEmptyResult(result any, query string) string {
+	var isEmpty bool
+
+	switch v := result.(type) {
+	case model.Matrix:
+		isEmpty = len(v) == 0
+	case model.Vector:
+		isEmpty = len(v) == 0
+	case *model.Scalar:
+		isEmpty = v == nil
+	case *model.String:
+		isEmpty = v == nil
+	default:
+		return ""
+	}
+
+	if isEmpty {
+		return fmt.Sprintf("The query '%s' executed successfully but returned no data. "+
+			"This could mean: (1) the metric does not exist, (2) the metric exists but has no data for the specified time range, "+
+			"(3) the label filters are too restrictive, or (4) there's no data matching your query conditions. "+
+			"You can use the 'list_metrics' tool to see all available metrics, or try adjusting the time range or label filters.",
+			query)
+	}
+
+	return ""
+}
+
 func (p *RealLoader) ListMetrics(ctx context.Context) ([]string, error) {
 	labelValues, _, err := p.client.LabelValues(ctx, "__name__", []string{}, time.Now().Add(-ListMetricsTimeRange), time.Now())
 	if err != nil {
@@ -83,18 +150,24 @@ func (p *RealLoader) ExecuteRangeQuery(ctx context.Context, query string, start,
 
 	result, warnings, err := p.client.QueryRange(ctx, query, r, v1.WithTimeout(DefaultQueryTimeout))
 	if err != nil {
-		return nil, fmt.Errorf("error executing range query: %w", err)
+		return nil, makeLLMFriendlyError(err, query)
 	}
 
 	response := map[string]any{
 		"resultType": result.Type().String(),
 		"result":     result,
 	}
 
+	// Add warnings from Prometheus
 	if len(warnings) > 0 {
 		response["warnings"] = warnings
 	}
 
+	// Check for empty results and add guidance separately
+	if emptyWarning := checkEmptyResult(result, query); emptyWarning != "" {
+		response["emptyResultGuidance"] = emptyWarning
+	}
+
 	return response, nil
 }
 
@@ -111,17 +184,23 @@ func (p *RealLoader) ExecuteInstantQuery(ctx context.Context, query string, ts t
 
 	result, warnings, err := p.client.Query(ctx, query, ts)
 	if err != nil {
-		return nil, fmt.Errorf("error executing instant query: %w", err)
+		return nil, makeLLMFriendlyError(err, query)
 	}
 
 	response := map[string]any{
 		"resultType": result.Type().String(),
 		"result":     result,
 	}
 
+	// Add warnings from Prometheus
 	if len(warnings) > 0 {
 		response["warnings"] = warnings
 	}
 
+	// Check for empty results and add guidance separately
+	if emptyWarning := checkEmptyResult(result, query); emptyWarning != "" {
+		response["emptyResultGuidance"] = emptyWarning
+	}
+
 	return response, nil
 }

pkg/prometheus/loader_test.go

@@ -0,0 +1,227 @@
+package prometheus
+
+import (
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/prometheus/common/model"
+)
+
+func TestMakeLLMFriendlyError(t *testing.T) {
+	tests := []struct {
+		name           string
+		originalError  error
+		query          string
+		expectedSubstr []string // substrings that should be in the error message
+	}{
+		{
+			name:          "parse error",
+			originalError: errors.New("parse error: unexpected character"),
+			query:         "up{invalid",
+			expectedSubstr: []string{
+				"syntax error",
+				"up{invalid",
+				"check the query syntax",
+			},
+		},
+		{
+			name:          "bad_data error",
+			originalError: errors.New("bad_data: invalid expression"),
+			query:         "rate(http[5m])",
+			expectedSubstr: []string{
+				"syntax error",
+				"rate(http[5m])",
+				"correctly formatted",
+			},
+		},
+		{
+			name:          "unknown function",
+			originalError: errors.New("unknown function: foobar"),
+			query:         "foobar(up)",
+			expectedSubstr: []string{
+				"unknown function",
+				"foobar(up)",
+				"function name is correct",
+			},
+		},
+		{
+			name:          "timeout error",
+			originalError: errors.New("query timeout exceeded"),
+			query:         "sum(rate(http_requests_total[5m])) by (job)",
+			expectedSubstr: []string{
+				"timed out",
+				"sum(rate(http_requests_total[5m])) by (job)",
+				"time range",
+				"step size",
+			},
+		},
+		{
+			name:          "deadline exceeded",
+			originalError: errors.New("context deadline exceeded"),
+			query:         "up",
+			expectedSubstr: []string{
+				"timed out",
+				"up",
+				"reducing the time range",
+			},
+		},
+		{
+			name:          "connection refused",
+			originalError: errors.New("connection refused"),
+			query:         "up",
+			expectedSubstr: []string{
+				"cannot connect",
+				"Prometheus server is running",
+			},
+		},
+		{
+			name:          "no such host",
+			originalError: errors.New("no such host: prometheus.example.com"),
+			query:         "up",
+			expectedSubstr: []string{
+				"cannot connect",
+				"Prometheus server is running",
+			},
+		},
+		{
+			name:          "generic error",
+			originalError: errors.New("some other error"),
+			query:         "up",
+			expectedSubstr: []string{
+				"up",
+				"some other error",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := makeLLMFriendlyError(tt.originalError, tt.query)
+			if result == nil {
+				t.Fatalf("expected error, got nil")
+			}
+
+			resultMsg := result.Error()
+			for _, substr := range tt.expectedSubstr {
+				if !strings.Contains(resultMsg, substr) {
+					t.Errorf("expected error to contain %q, got: %s", substr, resultMsg)
+				}
+			}
+		})
+	}
+}
+
+func TestMakeLLMFriendlyError_NilError(t *testing.T) {
+	result := makeLLMFriendlyError(nil, "up")
+	if result != nil {
+		t.Errorf("expected nil error for nil input, got: %v", result)
+	}
+}
+
+func TestCheckEmptyResult(t *testing.T) {
+	tests := []struct {
+		name           string
+		result         any
+		query          string
+		expectWarning  bool
+		expectedSubstr []string
+	}{
+		{
+			name:          "empty matrix",
+			result:        model.Matrix{},
+			query:         "nonexistent_metric",
+			expectWarning: true,
+			expectedSubstr: []string{
+				"nonexistent_metric",
+				"returned no data",
+				"metric does not exist",
+				"no data for the specified time range",
+				"list_metrics",
+			},
+		},
+		{
+			name:          "empty vector",
+			result:        model.Vector{},
+			query:         "up{job=\"missing\"}",
+			expectWarning: true,
+			expectedSubstr: []string{
+				"up{job=\"missing\"}",
+				"returned no data",
+				"label filters are too restrictive",
+			},
+		},
+		{
+			name: "non-empty matrix",
+			result: model.Matrix{
+				&model.SampleStream{
+					Metric: model.Metric{"__name__": "up"},
+					Values: []model.SamplePair{{Timestamp: 0, Value: 1}},
+				},
+			},
+			query:         "up",
+			expectWarning: false,
+		},
+		{
+			name: "non-empty vector",
+			result: model.Vector{
+				&model.Sample{
+					Metric:    model.Metric{"__name__": "up"},
+					Timestamp: 0,
+					Value:     1,
+				},
+			},
+			query:         "up",
+			expectWarning: false,
+		},
+		{
+			name:          "nil scalar",
+			result:        (*model.Scalar)(nil),
+			query:         "scalar(nonexistent)",
+			expectWarning: true,
+		},
+		{
+			name:          "valid scalar",
+			result:        &model.Scalar{Value: 1, Timestamp: 0},
+			query:         "scalar(up)",
+			expectWarning: false,
+		},
+		{
+			name:          "nil string",
+			result:        (*model.String)(nil),
+			query:         "string_metric",
+			expectWarning: true,
+		},
+		{
+			name:          "valid string",
+			result:        &model.String{Value: "test", Timestamp: 0},
+			query:         "string_metric",
+			expectWarning: false,
+		},
+		{
+			name:          "unknown type",
+			result:        "unknown",
+			query:         "up",
+			expectWarning: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			warning := checkEmptyResult(tt.result, tt.query)
+
+			if tt.expectWarning {
+				if warning == "" {
+					t.Errorf("expected warning for empty result, got none")
+				}
+				for _, substr := range tt.expectedSubstr {
+					if !strings.Contains(warning, substr) {
+						t.Errorf("expected warning to contain %q, got: %s", substr, warning)
+					}
+				}
+			} else if warning != "" {
+				t.Errorf("expected no warning, got: %s", warning)
+			}
+		})
+	}
+}