Skip to content

Commit

Permalink
reset the basemap when the labelsbuilder is reset (#11771)
Browse files Browse the repository at this point in the history
This PR fixes an issue where the `label_format` stage sometimes doesn't
populate labels when it should.

The label `baz` exists on all log lines but is only populated on the
first line.

![image](https://github.com/grafana/loki/assets/1413241/86585a94-937f-4e07-9657-b94699ce8586)

The issue is this call to
[IntoMap](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/fmt.go#L403).
It tries to avoid copying the map multiple times
[here](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/labels.go#L482)
but the `baseMap` variable isn't reset when the labels builder is reset.
As a result, calls to to `IntoMap` after the builder has been reset mean
that `!b.hasDel() && !b.hasAdd() && !b.HasErr()` evaluates to `true` but
`b.baseMap == nil` also evaluates to `true` so no items are copied.

This PR makes it so that `b.baseMap` is reset along with the rest of the
builder's state in `Reset`
  • Loading branch information
MasslessParticle authored Jan 24, 2024
1 parent 06bb209 commit 2c9482d
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 4 deletions.
9 changes: 5 additions & 4 deletions pkg/logql/log/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ type BaseLabelsBuilder struct {
errDetails string

groups []string
baseMap map[string]string
parserKeyHints ParserHint // label key hints for metric queries that allows to limit parser extractions to only this list of labels.
without, noLabels bool
referencedStructuredMetadata bool
Expand All @@ -146,7 +147,6 @@ type BaseLabelsBuilder struct {
// LabelsBuilder is the same as labels.Builder but tailored for this package.
type LabelsBuilder struct {
base labels.Labels
baseMap map[string]string
buf labels.Labels
currentResult LabelsResult
groupedResult LabelsResult
Expand Down Expand Up @@ -211,6 +211,7 @@ func (b *BaseLabelsBuilder) Reset() {
}
b.err = ""
b.errDetails = ""
b.baseMap = nil
b.parserKeyHints.Reset()
}

Expand Down Expand Up @@ -481,9 +482,9 @@ func (b *LabelsBuilder) IntoMap(m map[string]string) {
if !b.hasDel() && !b.hasAdd() && !b.HasErr() {
if b.baseMap == nil {
b.baseMap = b.base.Map()
for k, v := range b.baseMap {
m[k] = v
}
}
for k, v := range b.baseMap {
m[k] = v
}
return
}
Expand Down
58 changes: 58 additions & 0 deletions pkg/logql/log/labels_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,64 @@ func TestLabelsBuilder_LabelsError(t *testing.T) {
require.Equal(t, labels.FromStrings("already", "in"), lbs)
}

func TestLabelsBuilder_IntoMap(t *testing.T) {
strs := []string{
"namespace", "loki",
"job", "us-central1/loki",
"cluster", "us-central1",
"ToReplace", "text",
}
lbs := labels.FromStrings(strs...)

t.Run("it still copies the map after a Reset", func(t *testing.T) {
b := NewBaseLabelsBuilder().ForLabels(lbs, lbs.Hash())

m := map[string]string{}
b.IntoMap(m)

require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m)

b.Reset()

m2 := map[string]string{}
b.IntoMap(m2)
require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m2)
})

t.Run("it can copy the map several times", func(t *testing.T) {
b := NewBaseLabelsBuilder().ForLabels(lbs, lbs.Hash())

m := map[string]string{}
b.IntoMap(m)

require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m)

m2 := map[string]string{}
b.IntoMap(m2)
require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m2)
})
}

func TestLabelsBuilder_LabelsResult(t *testing.T) {
strs := []string{
"namespace", "loki",
Expand Down

0 comments on commit 2c9482d

Please sign in to comment.