WIP: Upgrade google.golang.org/grpc to v1.69.0

[aknuds1]

What this PR does

TODO:

• Try using memory pooling when unmarshalling storepb.SeriesResponse in the querier, since it currently leads to huge memory utilization bursts.
• Fix memory usage in distributor.receiveResponse (QueryStreamResponse unmarshalling, stop holding on to unmarshalling buffer)

Which issue(s) this PR fixes or relates to

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[francoposa]

looked at test TestDistributor_QueryStream_ShouldReturnErrorIfMaxSeriesPerQueryLimitIsReached since it had a nil pointer dereference, - lot of the other failures are just a bad equality assert on one of the new struct types.

This new
hashToChunkseries map gets read from before it's written to, without a nil guard, so there's a panic in line 341

key := mimirpb.FromLabelAdaptersToKeyString(series.Labels)
existing := hashToChunkseries[key]
existing.Labels = series.Labels   // panic: existing is nil

[aknuds1]

I just fixed failures in pkg/distributor/query_test.go. I hadn't noticed that segfault though. I'll try to fix it tomorrow.

[francoposa]

this patch fixes that test, but it looks like there's the same bug below it for the hashToTimeSeries that is not getting caught by a test.

diff --git a/pkg/distributor/query.go b/pkg/distributor/query.go
index 2b766f30f4..41d83b124f 100644
--- a/pkg/distributor/query.go
+++ b/pkg/distributor/query.go
@@ -337,15 +337,20 @@ func (d *Distributor) queryIngesterStream(ctx context.Context, replicationSets [
 		for _, batch := range res.chunkseriesBatches {
 			for _, series := range batch {
 				key := mimirpb.FromLabelAdaptersToKeyString(series.Labels)
-				existing := hashToChunkseries[key]
-				existing.Labels = series.Labels
+				if existing, ok := hashToChunkseries[key]; ok {
+					existing.Labels = series.Labels
 
-				numPotentialChunks := len(existing.Chunks) + len(series.Chunks)
-				existing.Chunks = ingester_client.AccumulateChunks(existing.Chunks, series.Chunks)
+					numPotentialChunks := len(existing.Chunks) + len(series.Chunks)
+					existing.Chunks = ingester_client.AccumulateChunks(existing.Chunks, series.Chunks)
 
-				deduplicatedChunks += numPotentialChunks - len(existing.Chunks)
-				totalChunks += len(series.Chunks)
-				hashToChunkseries[key] = existing
+					deduplicatedChunks += numPotentialChunks - len(existing.Chunks)
+					totalChunks += len(series.Chunks)
+					hashToChunkseries[key] = existing
+				} else {
+					// no existing series to dedupe
+					totalChunks += len(series.Chunks)
+					hashToChunkseries[key] = series
+				}
 			}
 		}

[aknuds1]

I pushed a simple fix, that just reflects the change in the PR (i.e. that client.TimeSeriesChunk is wrapped by client.CustomTimeSeriesChunk). The problem is that the client.CustomTimeSeriesChunk isn't valid by itself (unlike client.TimeSeriesChunk), you need to initialize it with a non-nil TimeSeriesChunk field.

[aknuds1]

Do you still think there's a bug not getting caught?