Skip to content

Commit

Permalink
Do not send back non-indexed labels for each entry in query response (#…
Browse files Browse the repository at this point in the history
…10090)

**What this PR does / why we need it**:
Since the non-indexed labels are part of the returned labels results, we
don't need to send them also along with each returned entry. This PR
will:
-  Reduce the size of the response payload
- Solve a problem in Grafana only showing the last log line

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [x] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)
  • Loading branch information
salvacorts authored Jul 28, 2023
1 parent cbb272d commit 24fd566
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 13 deletions.
3 changes: 2 additions & 1 deletion pkg/chunkenc/memchunk.go
Original file line number Diff line number Diff line change
Expand Up @@ -1591,9 +1591,10 @@ func (e *entryBufferedIterator) Next() bool {

e.stats.AddPostFilterLines(1)
e.currLabels = lbs
e.cur.NonIndexedLabels = logproto.FromLabelsToLabelAdapters(e.currNonIndexedLabels)
e.cur.Timestamp = time.Unix(0, e.currTs)
e.cur.Line = string(newLine)
// There is no need to send back the non-indexed labels, as they are already part of the labels results
// e.cur.NonIndexedLabels = logproto.FromLabelsToLabelAdapters(e.currNonIndexedLabels)
return true
}
return false
Expand Down
19 changes: 13 additions & 6 deletions pkg/chunkenc/memchunk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,12 @@ func TestBlock(t *testing.T) {
e := it.Entry()
require.Equal(t, cases[idx].ts, e.Timestamp.UnixNano())
require.Equal(t, cases[idx].str, e.Line)
require.Empty(t, e.NonIndexedLabels)
if chunkFormat < chunkFormatV4 {
require.Empty(t, e.NonIndexedLabels)
require.Equal(t, labels.EmptyLabels().String(), it.Labels())
} else {
require.Equal(t, push.LabelsAdapter(cases[idx].lbs), e.NonIndexedLabels)
expectedLabels := logproto.FromLabelAdaptersToLabels(cases[idx].lbs).String()
require.Equal(t, expectedLabels, it.Labels())
}
idx++
}
Expand Down Expand Up @@ -434,10 +436,11 @@ func TestSerialization(t *testing.T) {
e := it.Entry()
require.Equal(t, int64(i), e.Timestamp.UnixNano())
require.Equal(t, strconv.Itoa(i), e.Line)
require.Nil(t, e.NonIndexedLabels)
if appendWithNonIndexedLabels && testData.chunkFormat >= chunkFormatV4 {
require.Equal(t, push.LabelsAdapter{{Name: "foo", Value: strconv.Itoa(i)}}, e.NonIndexedLabels)
require.Equal(t, labels.FromStrings("foo", strconv.Itoa(i)).String(), it.Labels())
} else {
require.Nil(t, e.NonIndexedLabels)
require.Equal(t, labels.EmptyLabels().String(), it.Labels())
}
}
require.NoError(t, it.Error())
Expand All @@ -458,9 +461,9 @@ func TestSerialization(t *testing.T) {
require.Equal(t, int64(i), s.Timestamp)
require.Equal(t, 1., s.Value)
if appendWithNonIndexedLabels && testData.chunkFormat >= chunkFormatV4 {
require.Equal(t, fmt.Sprintf(`{foo="%d"}`, i), sampleIt.Labels())
require.Equal(t, labels.FromStrings("foo", strconv.Itoa(i)).String(), sampleIt.Labels())
} else {
require.Equal(t, "{}", sampleIt.Labels())
require.Equal(t, labels.EmptyLabels().String(), sampleIt.Labels())
}
}
require.NoError(t, sampleIt.Error())
Expand Down Expand Up @@ -1765,6 +1768,10 @@ func TestMemChunk_IteratorWithNonIndexedLabels(t *testing.T) {
e := it.Entry()
lines = append(lines, e.Line)
streams = append(streams, it.Labels())

// We don't want to send back the non-indexed labels since
// they are already part of the returned labels.
require.Empty(t, e.NonIndexedLabels)
}
assert.ElementsMatch(t, tc.expectedLines, lines)
assert.ElementsMatch(t, tc.expectedStreams, streams)
Expand Down
7 changes: 4 additions & 3 deletions pkg/chunkenc/unordered.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +279,10 @@ func (hb *unorderedHeadBlock) Iterator(
}

stream.Entries = append(stream.Entries, logproto.Entry{
Timestamp: time.Unix(0, ts),
Line: newLine,
NonIndexedLabels: logproto.FromLabelsToLabelAdapters(hb.symbolizer.Lookup(nonIndexedLabelsSymbols)),
Timestamp: time.Unix(0, ts),
Line: newLine,
// There is no need to send back the non-indexed labels, as they are already part of the labels results
// NonIndexedLabels: logproto.FromLabelsToLabelAdapters(hb.symbolizer.Lookup(nonIndexedLabelsSymbols)),
})
return nil
},
Expand Down
6 changes: 3 additions & 3 deletions pkg/chunkenc/unordered_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ func iterEq(t *testing.T, exp []entry, got iter.EntryIterator) {
var i int
for got.Next() {
require.Equal(t, logproto.Entry{
Timestamp: time.Unix(0, exp[i].t),
Line: exp[i].s,
NonIndexedLabels: logproto.FromLabelsToLabelAdapters(exp[i].nonIndexedLabels),
Timestamp: time.Unix(0, exp[i].t),
Line: exp[i].s,
}, got.Entry())
require.Equal(t, exp[i].nonIndexedLabels.String(), got.Labels())
i++
}
require.Equal(t, i, len(exp))
Expand Down

0 comments on commit 24fd566

Please sign in to comment.