refactor: extract Grafana Client creation from context functions (#127)

* refactor: extract Grafana Client creation from context functions

There was a bunch of duplicated code when creating the Grafana client from
context, and I want to be able to do this in tests too, so this just refactors
the code for later.

* test: add test case for finding log error patterns (#128)

* Add log lines when creating Sift investigation

Should help debug issues when running Sift tools.

* Include Grafana client in cloud test context

Using the refactored NewGrafanaClient function from
PR #127.

* Fetch multiple investigations in tests, in case the first one has no analyses

This can happen if, for example, an investigation was run with a scope
that didn't find any logs, and so no analyses were created.

* Add test for finding error pattern logs

This relies on a new Loki datasource being added to the mcptests
instance, which points to our dev logs. We probably want to get rid of
this and use different logs; I'll look into how we can do that.

Author: sd2k

mcpgrafana.go

@@ -154,19 +154,18 @@ func makeBasePath(path string) string {
 	return strings.Join([]string{strings.TrimRight(path, "/"), "api"}, "/")
 }
 
-// ExtractGrafanaClientFromEnv is a StdioContextFunc that extracts Grafana configuration
-// from environment variables and injects a configured client into the context.
-var ExtractGrafanaClientFromEnv server.StdioContextFunc = func(ctx context.Context) context.Context {
+// NewGrafanaClient creates a Grafana client with the provided URL and API key,
+// configured to use the correct scheme and debug mode.
+func NewGrafanaClient(ctx context.Context, grafanaURL, apiKey string) *client.GrafanaHTTPAPI {
 	cfg := client.DefaultTransportConfig()
-	// Extract transport config from env vars, and set it on the context.
-	var grafanaURL string
+
 	var parsedURL *url.URL
-	var ok bool
 	var err error
-	if grafanaURL, ok = os.LookupEnv(grafanaURLEnvVar); ok {
+
+	if grafanaURL != "" {
 		parsedURL, err = url.Parse(grafanaURL)
 		if err != nil {
-			panic(fmt.Errorf("invalid %s: %w", grafanaURLEnvVar, err))
+			panic(fmt.Errorf("invalid Grafana URL: %w", err))
 		}
 		cfg.Host = parsedURL.Host
 		cfg.BasePath = makeBasePath(parsedURL.Path)
@@ -179,22 +178,33 @@ var ExtractGrafanaClientFromEnv server.StdioContextFunc = func(ctx context.Conte
 		parsedURL, _ = url.Parse(defaultGrafanaURL)
 	}
 
-	apiKey := os.Getenv(grafanaAPIEnvVar)
 	if apiKey != "" {
 		cfg.APIKey = apiKey
 	}
 
 	cfg.Debug = GrafanaDebugFromContext(ctx)
 
 	slog.Debug("Creating Grafana client", "url", parsedURL.Redacted(), "api_key_set", apiKey != "")
-	client := client.NewHTTPClientWithConfig(strfmt.Default, cfg)
-	return context.WithValue(ctx, grafanaClientKey{}, client)
+	return client.NewHTTPClientWithConfig(strfmt.Default, cfg)
+}
+
+// ExtractGrafanaClientFromEnv is a StdioContextFunc that extracts Grafana configuration
+// from environment variables and injects a configured client into the context.
+var ExtractGrafanaClientFromEnv server.StdioContextFunc = func(ctx context.Context) context.Context {
+	// Extract transport config from env vars
+	grafanaURL, ok := os.LookupEnv(grafanaURLEnvVar)
+	if !ok {
+		grafanaURL = defaultGrafanaURL
+	}
+	apiKey := os.Getenv(grafanaAPIEnvVar)
+
+	grafanaClient := NewGrafanaClient(ctx, grafanaURL, apiKey)
+	return context.WithValue(ctx, grafanaClientKey{}, grafanaClient)
 }
 
 // ExtractGrafanaClientFromHeaders is a HTTPContextFunc that extracts Grafana configuration
 // from request headers and injects a configured client into the context.
 var ExtractGrafanaClientFromHeaders server.HTTPContextFunc = func(ctx context.Context, req *http.Request) context.Context {
-	cfg := client.DefaultTransportConfig()
 	// Extract transport config from request headers, and set it on the context.
 	u, apiKey := urlAndAPIKeyFromHeaders(req)
 	uEnv, apiKeyEnv := urlAndAPIKeyFromEnv()
@@ -204,23 +214,9 @@ var ExtractGrafanaClientFromHeaders server.HTTPContextFunc = func(ctx context.Co
 	if apiKey == "" {
 		apiKey = apiKeyEnv
 	}
-	if u != "" {
-		if url, err := url.Parse(u); err == nil {
-			cfg.Host = url.Host
-			cfg.BasePath = makeBasePath(url.Path)
-			if url.Scheme == "http" {
-				cfg.Schemes = []string{"http"}
-			}
-		}
-	}
-	if apiKey != "" {
-		cfg.APIKey = apiKey
-	}
-
-	cfg.Debug = GrafanaDebugFromContext(ctx)
 
-	client := client.NewHTTPClientWithConfig(strfmt.Default, cfg)
-	return WithGrafanaClient(ctx, client)
+	grafanaClient := NewGrafanaClient(ctx, u, apiKey)
+	return WithGrafanaClient(ctx, grafanaClient)
 }
 
 // WithGrafanaClient sets the Grafana client in the context.

tools/cloud_testing_utils.go

@@ -11,11 +11,14 @@ import (
 	mcpgrafana "github.com/grafana/mcp-grafana"
 )
 
-// createCloudTestContext creates a context with Grafana URL and API key for cloud integration tests.
+// createCloudTestContext creates a context with a Grafana URL, Grafana API key and
+// Grafana client for cloud integration tests.
 // The test will be skipped if required environment variables are not set.
 // testName is used to customize the skip message (e.g. "OnCall", "Sift", "Incident")
 // urlEnv and apiKeyEnv specify the environment variable names for the Grafana URL and API key.
 func createCloudTestContext(t *testing.T, testName, urlEnv, apiKeyEnv string) context.Context {
+	ctx := context.Background()
+
 	grafanaURL := os.Getenv(urlEnv)
 	if grafanaURL == "" {
 		t.Skipf("%s environment variable not set, skipping cloud %s integration tests", urlEnv, testName)
@@ -26,9 +29,11 @@ func createCloudTestContext(t *testing.T, testName, urlEnv, apiKeyEnv string) co
 		t.Skipf("%s environment variable not set, skipping cloud %s integration tests", apiKeyEnv, testName)
 	}
 
-	ctx := context.Background()
+	client := mcpgrafana.NewGrafanaClient(ctx, grafanaURL, grafanaApiKey)
+
 	ctx = mcpgrafana.WithGrafanaURL(ctx, grafanaURL)
 	ctx = mcpgrafana.WithGrafanaAPIKey(ctx, grafanaApiKey)
+	ctx = mcpgrafana.WithGrafanaClient(ctx, client)
 
 	return ctx
 }

tools/sift.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
 	"time"
 
@@ -288,6 +289,7 @@ func findErrorPatternLogs(ctx context.Context, args FindErrorPatternLogsParams)
 	}
 
 	// Get all analyses from the completed investigation
+	slog.Debug("Getting analyses", "investigation_id", completedInvestigation.ID)
 	analyses, err := client.getSiftAnalyses(ctx, completedInvestigation.ID)
 	if err != nil {
 		return nil, fmt.Errorf("getting analyses: %w", err)
@@ -305,6 +307,7 @@ func findErrorPatternLogs(ctx context.Context, args FindErrorPatternLogsParams)
 	if errorPatternLogsAnalysis == nil {
 		return nil, fmt.Errorf("ErrorPatternLogs analysis not found in investigation %s", completedInvestigation.ID)
 	}
+	slog.Debug("Found ErrorPatternLogs analysis", "analysis_id", errorPatternLogsAnalysis.ID)
 
 	datasourceUID := completedInvestigation.Datasources.LokiDatasource.UID
 
@@ -485,10 +488,12 @@ func (c *siftClient) createSiftInvestigation(ctx context.Context, investigation
 		return nil, fmt.Errorf("marshaling investigation: %w", err)
 	}
 
+	slog.Debug("Creating investigation", "payload", string(jsonData))
 	buf, err := c.makeRequest(ctx, "POST", "/api/plugins/grafana-ml-app/resources/sift/api/v1/investigations", jsonData)
 	if err != nil {
 		return nil, err
 	}
+	slog.Debug("Investigation created", "response", string(buf))
 
 	investigationResponse := struct {
 		Status string        `json:"status"`
@@ -512,6 +517,7 @@ func (c *siftClient) createSiftInvestigation(ctx context.Context, investigation
 		case <-timeout:
 			return nil, fmt.Errorf("timeout waiting for investigation completion after 5 minutes")
 		case <-ticker.C:
+			slog.Debug("Polling investigation status", "investigation_id", investigationResponse.Data.ID)
 			investigation, err := c.getSiftInvestigation(ctx, investigationResponse.Data.ID)
 			if err != nil {
 				return nil, err

tools/sift_cloud_test.go

@@ -11,6 +11,7 @@ package tools
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -49,11 +50,19 @@ func TestCloudSiftInvestigations(t *testing.T) {
 	})
 
 	// Get an investigation ID from the list to test getting a specific investigation
-	investigations, err := listSiftInvestigations(ctx, ListSiftInvestigationsParams{Limit: 1})
+	investigations, err := listSiftInvestigations(ctx, ListSiftInvestigationsParams{Limit: 10})
 	require.NoError(t, err, "Should not error when listing investigations")
 	require.NotEmpty(t, investigations, "Should have at least one investigation to test with")
 
-	investigationID := investigations[0].ID.String()
+	// Find an investigation with at least one analysis.
+	var investigationID string
+	for _, investigation := range investigations {
+		if len(investigation.Analyses.Items) > 0 {
+			investigationID = investigation.ID.String()
+			break
+		}
+	}
+	require.NotEmpty(t, investigationID, "Should have at least one investigation with at least one analysis")
 
 	// Test getting a specific investigation
 	t.Run("get specific investigation", func(t *testing.T) {
@@ -105,4 +114,25 @@ func TestCloudSiftInvestigations(t *testing.T) {
 		assert.NotEmpty(t, analysis.InvestigationID, "Analysis should have an investigation ID")
 		assert.NotNil(t, analysis.Result, "Analysis should have a result")
 	})
+
+	t.Run("find error patterns", func(t *testing.T) {
+		// Find error patterns
+		analysis, err := findErrorPatternLogs(ctx, FindErrorPatternLogsParams{
+			Name: "Test Sift",
+			Labels: map[string]string{
+				"namespace": "hosted-grafana",
+				"cluster":   "dev-eu-west-2",
+				"slug":      "mcptests",
+			},
+			Start: time.Now().Add(-5 * time.Minute),
+			End:   time.Now(),
+		})
+		require.NoError(t, err, "Should not error when finding error patterns")
+		assert.NotNil(t, analysis, "Result should not be nil")
+
+		// Verify all required fields are present
+		assert.NotEmpty(t, analysis.Name, "Analysis should have a name")
+		assert.NotEmpty(t, analysis.InvestigationID, "Analysis should have an investigation ID")
+		assert.NotEmpty(t, analysis.Result.Message, "Analysis  should have a message")
+	})
 }