Remove unnecessary debuginfod client raw files cache & address several review comments

Author: marcsanmi

.mockery.yaml

@@ -63,3 +63,4 @@ packages:
   github.com/grafana/pyroscope/pkg/experiment/symbolizer:
     interfaces:
       DebuginfodClient:
+      DebugInfoStore:

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/cespare/xxhash/v2 v2.3.0
 	github.com/colega/zeropool v0.0.0-20230505084239-6fb4a4f75381
 	github.com/dennwc/varint v1.0.0
-	github.com/dgraph-io/ristretto v0.2.0
+	github.com/dgraph-io/ristretto/v2 v2.2.0
 	github.com/dgryski/go-groupvarint v0.0.0-20230630160417-2bfb7969fb3c
 	github.com/dolthub/swiss v0.2.1
 	github.com/drone/envsubst v1.0.3

go.sum

@@ -197,10 +197,10 @@ github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dennwc/varint v1.0.0 h1:kGNFFSSw8ToIy3obO/kKr8U9GZYUAxQEVuix4zfDWzE=
 github.com/dennwc/varint v1.0.0/go.mod h1:hnItb35rvZvJrbTALZtY/iQfDs48JKRG1RPpgziApxA=
-github.com/dgraph-io/ristretto v0.2.0 h1:XAfl+7cmoUDWW/2Lx8TGZQjjxIQ2Ley9DSf52dru4WE=
-github.com/dgraph-io/ristretto v0.2.0/go.mod h1:8uBHCU/PBV4Ag0CJrP47b9Ofby5dqWNh4FicAdoqFNU=
-github.com/dgryski/go-farm v0.0.0-20200201041132-a6ae2369ad13 h1:fAjc9m62+UWV/WAFKLNi6ZS0675eEUC9y3AlwSbQu1Y=
-github.com/dgryski/go-farm v0.0.0-20200201041132-a6ae2369ad13/go.mod h1:SqUrOPUnsFjfmXRMNPybcSiG0BgUW2AuFH8PAnS2iTw=
+github.com/dgraph-io/ristretto/v2 v2.2.0 h1:bkY3XzJcXoMuELV8F+vS8kzNgicwQFAaGINAEJdWGOM=
+github.com/dgraph-io/ristretto/v2 v2.2.0/go.mod h1:RZrm63UmcBAaYWC1DotLYBmTvgkrs0+XhBd7Npn7/zI=
+github.com/dgryski/go-farm v0.0.0-20240924180020-3414d57e47da h1:aIftn67I1fkbMa512G+w+Pxci9hJPB8oMnkcP3iZF38=
+github.com/dgryski/go-farm v0.0.0-20240924180020-3414d57e47da/go.mod h1:SqUrOPUnsFjfmXRMNPybcSiG0BgUW2AuFH8PAnS2iTw=
 github.com/dgryski/go-groupvarint v0.0.0-20230630160417-2bfb7969fb3c h1:cHaw4wmusVzAZLEPWOCCGCfu6UvFXx9UboCHQCnjvxY=
 github.com/dgryski/go-groupvarint v0.0.0-20230630160417-2bfb7969fb3c/go.mod h1:MlkUQveSLEDbIgq2r1e++tSf0zfzU9mQpa9Qkczl+9Y=
 github.com/digitalocean/godo v1.109.0 h1:4W97RJLJSUQ3veRZDNbp1Ol3Rbn6Lmt9bKGvfqYI5SU=

pkg/experiment/symbolizer/debuginfo_store.go

@@ -16,7 +16,7 @@ type DebugInfoStoreConfig struct {
 	Storage objstoreclient.Config `yaml:"storage"`
 }
 
-// DebugInfoCache handles caching of debug info files
+// DebugInfoStore handles caching of debug info files
 type DebugInfoStore interface {
 	Get(ctx context.Context, buildID string) (io.ReadCloser, error)
 	Put(ctx context.Context, buildID string, reader io.Reader) error

pkg/experiment/symbolizer/debuginfod_client.go

@@ -13,7 +13,6 @@ import (
 	"regexp"
 	"time"
 
-	"github.com/dgraph-io/ristretto"
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
 	"github.com/grafana/dskit/backoff"
@@ -58,9 +57,6 @@ type DebuginfodHTTPClient struct {
 	metrics *Metrics
 	logger  log.Logger
 
-	// In-memory cache of build IDs to debug info data
-	cache *ristretto.Cache
-
 	// Used to deduplicate concurrent requests for the same build ID
 	group singleflight.Group
 }
@@ -77,13 +73,6 @@ func NewDebuginfodClient(logger log.Logger, baseURL string, metrics *Metrics) (*
 			MaxBackoff: 10 * time.Second,
 			MaxRetries: 3,
 		},
-		CacheConfig: struct {
-			MaxSizeBytes int64
-			NumCounters  int64
-		}{
-			MaxSizeBytes: 2 << 30, // 2GB default
-			NumCounters:  1e7,     // 10M default
-		},
 	}, metrics)
 }
 
@@ -111,25 +100,6 @@ func NewDebuginfodClientWithConfig(logger log.Logger, cfg DebuginfodClientConfig
 		}
 	}
 
-	// Use default cache config if not specified
-	if cfg.CacheConfig.MaxSizeBytes == 0 {
-		cfg.CacheConfig.MaxSizeBytes = 2 << 30 // 2GB default
-	}
-	if cfg.CacheConfig.NumCounters == 0 {
-		cfg.CacheConfig.NumCounters = 1e7 // 10M default
-	}
-
-	// Create cache
-	cache, err := ristretto.NewCache(&ristretto.Config{
-		NumCounters: cfg.CacheConfig.NumCounters,
-		MaxCost:     cfg.CacheConfig.MaxSizeBytes,
-		BufferItems: 64, // number of keys per Get buffer
-	})
-
-	if err != nil {
-		return nil, fmt.Errorf("failed to create debuginfod cache: %w", err)
-	}
-
 	return &DebuginfodHTTPClient{
 		cfg: DebuginfodClientConfig{
 			BaseURL:       cfg.BaseURL,
@@ -140,7 +110,6 @@ func NewDebuginfodClientWithConfig(logger log.Logger, cfg DebuginfodClientConfig
 			CacheConfig:   cfg.CacheConfig,
 		},
 		metrics: metrics,
-		cache:   cache,
 		logger:  logger,
 	}, nil
 }
@@ -155,22 +124,22 @@ func (c *DebuginfodHTTPClient) FetchDebuginfo(ctx context.Context, buildID strin
 		c.metrics.debuginfodRequestDuration.WithLabelValues(status).Observe(time.Since(start).Seconds())
 	}()
 
+	level.Debug(c.logger).Log(
+		"msg", "symbolizer: starting debuginfod fetch",
+		"build_id", buildID,
+	)
+
 	sanitizedBuildID, err := sanitizeBuildID(buildID)
 	if err != nil {
 		status = StatusErrorInvalidID
 		return nil, err
 	}
 
-	// Check in-memory cache first
-	if c.cache != nil {
-		if data, found := c.cache.Get(sanitizedBuildID); found {
-			status = StatusCacheHit
-			return io.NopCloser(bytes.NewReader(data.([]byte))), nil
-		}
-	}
-
 	// Check if there's already a request in flight for this build ID
-	// This prevents duplicate requests for the same build ID
+	level.Debug(c.logger).Log(
+		"msg", "symbolizer: making debuginfod request",
+		"build_id", sanitizedBuildID,
+	)
 	v, err, _ := c.group.Do(sanitizedBuildID, func() (interface{}, error) {
 		return c.fetchDebugInfoWithRetries(ctx, sanitizedBuildID)
 	})
@@ -193,6 +162,11 @@ func (c *DebuginfodHTTPClient) FetchDebuginfo(ctx context.Context, buildID strin
 
 	data := v.([]byte)
 	c.metrics.debuginfodFileSize.Observe(float64(len(data)))
+	level.Debug(c.logger).Log(
+		"msg", "symbolizer: debuginfod fetch successful",
+		"build_id", sanitizedBuildID,
+		"size", len(data),
+	)
 	return io.NopCloser(bytes.NewReader(data)), nil
 }
 
@@ -255,7 +229,7 @@ func (c *DebuginfodHTTPClient) fetchDebugInfoWithRetries(ctx context.Context, sa
 		statusCode, isHTTPErr := isHTTPStatusError(err)
 		if isHTTPErr && statusCode == http.StatusNotFound {
 			lastErr = buildIDNotFoundError{buildID: sanitizedBuildID}
-			return true // Stop retrying on 404
+			return true
 		}
 
 		// Store the error for later reporting
@@ -287,19 +261,6 @@ func (c *DebuginfodHTTPClient) fetchDebugInfoWithRetries(ctx context.Context, sa
 		return nil, fmt.Errorf("failed to fetch debuginfo after %d attempts: %w", backOff.NumRetries(), lastErr)
 	}
 
-	// Store successful result in cache
-	if c.cache != nil {
-		// The cost is the size of the data in bytes
-		success := c.cache.Set(sanitizedBuildID, data, int64(len(data)))
-		if !success && c.logger != nil {
-			level.Warn(c.logger).Log(
-				"msg", "Failed to store debuginfo in cache",
-				"sanitizedBuildID", sanitizedBuildID,
-				"size", len(data),
-			)
-		}
-	}
-
 	return data, nil
 }
 
@@ -376,6 +337,10 @@ func isRetryableError(err error) bool {
 // It validates that the build ID contains only alphanumeric characters, underscores, and hyphens.
 // This prevents potential security issues like path traversal attacks.
 func sanitizeBuildID(buildID string) (string, error) {
+	if buildID == "" {
+		return "", invalidBuildIDError{buildID: buildID}
+	}
+
 	// Only allow alphanumeric characters, underscores, and hyphens
 	validBuildID := regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)
 	if !validBuildID.MatchString(buildID) {

pkg/experiment/symbolizer/debuginfod_client_test.go

@@ -123,47 +123,6 @@ func TestDebuginfodClient(t *testing.T) {
 	}
 }
 
-func TestDebuginfodClientCache(t *testing.T) {
-	// Create a test server that counts requests
-	requestCount := 0
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		requestCount++
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("mock debug info"))
-	}))
-	defer server.Close()
-
-	// Create a client with the test server URL
-	metrics := NewMetrics(prometheus.NewRegistry())
-	client, err := NewDebuginfodClient(log.NewNopLogger(), server.URL, metrics)
-	require.NoError(t, err)
-
-	// Make multiple requests with the same build ID
-	buildID := "cache-test-id"
-	ctx := context.Background()
-
-	// First request should hit the server
-	reader1, err := client.FetchDebuginfo(ctx, buildID)
-	require.NoError(t, err)
-	data1, err := io.ReadAll(reader1)
-	require.NoError(t, err)
-	reader1.Close()
-	assert.Equal(t, "mock debug info", string(data1))
-	assert.Equal(t, 1, requestCount)
-
-	// Wait for cache operations to complete
-	client.cache.Wait()
-
-	// Second request should use the cache
-	reader2, err := client.FetchDebuginfo(ctx, buildID)
-	require.NoError(t, err)
-	data2, err := io.ReadAll(reader2)
-	require.NoError(t, err)
-	reader2.Close()
-	assert.Equal(t, "mock debug info", string(data2))
-	assert.Equal(t, 1, requestCount, "Expected no additional HTTP requests")
-}
-
 func TestDebuginfodClientSingleflight(t *testing.T) {
 	// Create a test server that sleeps to simulate a slow response
 	requestCount := 0