[BugFix] Add object name to cache key

CHANGELOG.md

@@ -67,6 +67,7 @@ configurable via the throughput_bytes_slo field, and it will populate op="traces
 * [BUGFIX] Make comparison to nil symmetric [#4869](https://github.com/grafana/tempo/pull/4869) (@stoewer)
 * [BUGFIX] Fix behavior for queries like {.foo && true} and {.foo || false} [#4855](https://github.com/grafana/tempo/pull/4855) (@stoewer)
 * [BUGFIX] Fix performance bottleneck and file cleanup in block builder [#4550](https://github.com/grafana/tempo/pull/4550) (@mdisibio)
+* [BUGFIX] Add object name to cache key in ReadRange [#4982](https://github.com/grafana/tempo/pull/4982) (@joe-elliott)
 * [BUGFIX] TraceQL incorrect results for additional spanset filters after a select operation [#4600](https://github.com/grafana/tempo/pull/4600) (@mdisibio)
 * [BUGFIX] TraceQL metrics incorrect results for queries with multiple filters that reside in non-dedicated columns that also group by the same variable [#4887](https://github.com/grafana/tempo/pull/4887) (@mdisibio)
 * [BUGFIX] TraceQL results caching bug for floats ending in .0 [#4539](https://github.com/grafana/tempo/pull/4539) (@carles-grafana)

tempodb/backend/cache/cache.go

@@ -111,9 +111,7 @@ func (r *readerWriter) ReadRange(ctx context.Context, name string, keypath backe
 	var k string
 	cache := r.cacheFor(cacheInfo)
 	if cache != nil {
-		// cache key is tenantID:blockID:offset:length - file name is not needed in key
-		keyGen := keypath
-		keyGen = append(keyGen, strconv.Itoa(int(offset)), strconv.Itoa(len(buffer)))
+		keyGen := append(keypath, name, strconv.Itoa(int(offset)), strconv.Itoa(len(buffer)))
 		k = strings.Join(keyGen, ":")
 		b, found := cache.FetchKey(ctx, k)
 		if found {

tempodb/backend/cache/cache_test.go

@@ -25,6 +25,14 @@ func TestCacheFor(t *testing.T) {
 
 	rw := reader.(*readerWriter)
 
+	// test.MockProvider will return the same cache for all requests. we need
+	// to override individual caches so that the test can validate the cache returned
+	rw.footerCache = test.NewMockClient()
+	rw.columnIdxCache = test.NewMockClient()
+	rw.offsetIdxCache = test.NewMockClient()
+	rw.traceIDIdxCache = test.NewMockClient()
+	rw.bloomCache = test.NewMockClient()
+
 	testCases := []struct {
 		name          string
 		cacheInfo     *backend.CacheInfo
@@ -260,3 +268,40 @@ func TestList(t *testing.T) {
 		})
 	}
 }
+
+func TestCacheKeys(t *testing.T) {
+	provider := test.NewMockProvider()
+	reader, _, err := NewCache(&BloomConfig{
+		CacheMaxBlockAge:        time.Hour,
+		CacheMinCompactionLevel: 1,
+	}, nil, nil, provider, log.NewNopLogger())
+	require.NoError(t, err)
+
+	ctx := context.Background()
+	role := cache.RoleParquetFooter // role doesn't matter b/c the mock provider always returns the same cache
+
+	// Read : seed data at expected key
+	expectedKey := "bar:baz:foo" // keypath + object name
+	expectedData := []byte("test-read")
+	provider.CacheFor(role).Store(ctx, []string{expectedKey}, [][]byte{expectedData})
+
+	// make request and confirm it returns
+	actualReader, actualBytes, err := reader.Read(ctx, "foo", backend.KeyPath{"bar", "baz"}, &backend.CacheInfo{Role: role})
+	require.NoError(t, err)
+	require.Equal(t, len(expectedData), int(actualBytes))
+
+	actualData, err := io.ReadAll(actualReader)
+	require.NoError(t, err)
+	require.Equal(t, expectedData, actualData)
+
+	// ReadRange : seed data at expected key
+	expectedKey = "bar:baz:foo:10:10" // keypath + object name + offset + length
+	expectedData = []byte("test-range")
+	provider.CacheFor(role).Store(ctx, []string{expectedKey}, [][]byte{expectedData})
+
+	// make request and confirm it returns
+	actualBuffer := make([]byte, 10)
+	err = reader.ReadRange(ctx, "foo", backend.KeyPath{"bar", "baz"}, 10, actualBuffer, &backend.CacheInfo{Role: role})
+	require.NoError(t, err)
+	require.Equal(t, expectedData, actualBuffer)
+}