fixes chunk size method in facade #3981
Conversation
| @@ -111,6 +111,7 @@ type Chunk interface { | |||
| SampleIterator(ctx context.Context, from, through time.Time, extractor log.StreamSampleExtractor) iter.SampleIterator | |||
| // Returns the list of blocks in the chunks. | |||
| Blocks(mintT, maxtT time.Time) []Block | |||
| // Size returns the number of entries in a chunk | |||
There was a problem hiding this comment.
I'm confused by the comment ? Should it be the size of the chunk ?
There was a problem hiding this comment.
It should, but we've historically used it as the number of entries:
https://github.com/grafana/loki/blob/main/pkg/ingester/stream.go#L140-L143
https://github.com/grafana/loki/blob/main/pkg/ingester/flush.go#L398
There was a problem hiding this comment.
Should you fix the comment ? That's where I'm confused.
There was a problem hiding this comment.
I think the comment is fixed - we use chunkenc.Chunk's Size() method to give the number of entries. This is different from Cortex's Chunk type, which the Facade now correctly accounts for.
I can also change the method name, but I figured this was the least intrusive change.
I noticed we've misrepresented the
Chunk.Sizemethod across Loki & Cortex. Loki uses this to describe the chunk's entry count, while Cortex uses this to describe the byte size. You can see this by comparing the following two metrics:sum(rate(loki_ingester_chunk_size_bytes_sum[1m]))vs
sum(rate(cortex_chunk_store_stored_chunk_bytes_total[1m]))This hasn't hurt us asides from misaligning metrics, but it's good to fix.