From 2f107db11aed170b7debc5eccab1b4b2a134db9e Mon Sep 17 00:00:00 2001 From: Ravishankar Date: Sun, 25 Aug 2024 23:11:15 +0530 Subject: [PATCH 1/4] fix: allow rename of structuremetadata labels --- pkg/logql/log/fmt.go | 1 + pkg/storage/store_test.go | 56 +++++++++++++++++++++++++++--- pkg/storage/util_test.go | 72 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 4 deletions(-) diff --git a/pkg/logql/log/fmt.go b/pkg/logql/log/fmt.go index c69aa3d40bb01..e7c811c1436c5 100644 --- a/pkg/logql/log/fmt.go +++ b/pkg/logql/log/fmt.go @@ -407,6 +407,7 @@ func (lf *LabelsFormatter) Process(ts int64, l []byte, lbs *LabelsBuilder) ([]by lbs.SetErrorDetails(err.Error()) continue } + lbs.Del(f.Name) // Deleting to avoid duplication lbs.Set(ParsedLabel, f.Name, lf.buf.String()) } return l, true diff --git a/pkg/storage/store_test.go b/pkg/storage/store_test.go index 101c906b8b4fe..70cc95169eb07 100644 --- a/pkg/storage/store_test.go +++ b/pkg/storage/store_test.go @@ -234,12 +234,14 @@ func getLocalStore(path string, cm ClientMetrics) Store { func Test_store_SelectLogs(t *testing.T) { tests := []struct { - name string - req *logproto.QueryRequest - expected []logproto.Stream + name string + storeFixture *mockChunkStore + req *logproto.QueryRequest + expected []logproto.Stream }{ { "all", + storeFixture, newQuery("{foo=~\"ba.*\"}", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -304,6 +306,7 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter regex", + storeFixture, newQuery("{foo=~\"ba.*\"} |~ \"1|2|3\" !~ \"2|3\"", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -328,6 +331,7 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter matcher", + storeFixture, newQuery("{foo=\"bar\"}", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -363,6 +367,7 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter time", + storeFixture, newQuery("{foo=~\"ba.*\"}", from, from.Add(time.Millisecond), nil, nil), []logproto.Stream{ { @@ -387,6 +392,7 @@ func Test_store_SelectLogs(t *testing.T) { }, { "delete covers whole time range", + storeFixture, newQuery( "{foo=~\"ba.*\"}", from, @@ -434,6 +440,7 @@ func Test_store_SelectLogs(t *testing.T) { }, { "delete covers partial time range", + storeFixture, newQuery( "{foo=~\"ba.*\"}", from, @@ -492,12 +499,53 @@ func Test_store_SelectLogs(t *testing.T) { }, }, }, + { + "rename using label_format", + storeFixtureV4, + newQuery("{foo=\"glop\"} | label_format boo=\"z\" | boo=\"z\"", from, from.Add(6*time.Millisecond), nil, nil), + []logproto.Stream{ + { + Labels: "{boo=\"z\", foo=\"glop\"}", + Entries: []logproto.Entry{ + { + Timestamp: from.Add(2 * time.Millisecond), + Line: "3", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(3 * time.Millisecond), + Line: "4", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + + { + Timestamp: from.Add(4 * time.Millisecond), + Line: "5", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(5 * time.Millisecond), + Line: "6", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &LokiStore{ - Store: storeFixture, + Store: tt.storeFixture, cfg: Config{ MaxChunkBatchSize: 10, }, diff --git a/pkg/storage/util_test.go b/pkg/storage/util_test.go index 5ef02e74b1caf..bb8a61db33b33 100644 --- a/pkg/storage/util_test.go +++ b/pkg/storage/util_test.go @@ -402,5 +402,77 @@ var streamsFixture = []*logproto.Stream{ }, }, }, + { + Labels: "{foo=\"glop\"}", + Entries: []logproto.Entry{ + { + Timestamp: from.Add(2 * time.Millisecond), + Line: "3", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(3 * time.Millisecond), + Line: "4", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + + { + Timestamp: from.Add(4 * time.Millisecond), + Line: "5", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(5 * time.Millisecond), + Line: "6", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + }, + }, } var storeFixture = newMockChunkStore(chunkenc.ChunkFormatV3, chunkenc.UnorderedWithStructuredMetadataHeadBlockFmt, streamsFixture) + +var streamsFixtureV4 = []*logproto.Stream{ + { + Labels: "{foo=\"glop\"}", + Entries: []logproto.Entry{ + { + Timestamp: from.Add(2 * time.Millisecond), + Line: "3", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(3 * time.Millisecond), + Line: "4", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + + { + Timestamp: from.Add(4 * time.Millisecond), + Line: "5", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + { + Timestamp: from.Add(5 * time.Millisecond), + Line: "6", + StructuredMetadata: []logproto.LabelAdapter{ + {Name: "boo", Value: "a"}, + }, + }, + }, + }, +} +var storeFixtureV4 = newMockChunkStore(chunkenc.ChunkFormatV4, chunkenc.UnorderedWithStructuredMetadataHeadBlockFmt, streamsFixtureV4) From 0cce5a20fdcceadf6007a21de0ad488ede578084 Mon Sep 17 00:00:00 2001 From: Ravishankar Date: Thu, 5 Sep 2024 18:19:19 +0530 Subject: [PATCH 2/4] Revert "fix: allow rename of structuremetadata labels" This reverts commit 741adb2b072599084aba3c892f67242198b27784. --- pkg/logql/log/fmt.go | 1 - pkg/storage/store_test.go | 56 +++--------------------------- pkg/storage/util_test.go | 72 --------------------------------------- 3 files changed, 4 insertions(+), 125 deletions(-) diff --git a/pkg/logql/log/fmt.go b/pkg/logql/log/fmt.go index e7c811c1436c5..c69aa3d40bb01 100644 --- a/pkg/logql/log/fmt.go +++ b/pkg/logql/log/fmt.go @@ -407,7 +407,6 @@ func (lf *LabelsFormatter) Process(ts int64, l []byte, lbs *LabelsBuilder) ([]by lbs.SetErrorDetails(err.Error()) continue } - lbs.Del(f.Name) // Deleting to avoid duplication lbs.Set(ParsedLabel, f.Name, lf.buf.String()) } return l, true diff --git a/pkg/storage/store_test.go b/pkg/storage/store_test.go index 70cc95169eb07..101c906b8b4fe 100644 --- a/pkg/storage/store_test.go +++ b/pkg/storage/store_test.go @@ -234,14 +234,12 @@ func getLocalStore(path string, cm ClientMetrics) Store { func Test_store_SelectLogs(t *testing.T) { tests := []struct { - name string - storeFixture *mockChunkStore - req *logproto.QueryRequest - expected []logproto.Stream + name string + req *logproto.QueryRequest + expected []logproto.Stream }{ { "all", - storeFixture, newQuery("{foo=~\"ba.*\"}", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -306,7 +304,6 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter regex", - storeFixture, newQuery("{foo=~\"ba.*\"} |~ \"1|2|3\" !~ \"2|3\"", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -331,7 +328,6 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter matcher", - storeFixture, newQuery("{foo=\"bar\"}", from, from.Add(6*time.Millisecond), nil, nil), []logproto.Stream{ { @@ -367,7 +363,6 @@ func Test_store_SelectLogs(t *testing.T) { }, { "filter time", - storeFixture, newQuery("{foo=~\"ba.*\"}", from, from.Add(time.Millisecond), nil, nil), []logproto.Stream{ { @@ -392,7 +387,6 @@ func Test_store_SelectLogs(t *testing.T) { }, { "delete covers whole time range", - storeFixture, newQuery( "{foo=~\"ba.*\"}", from, @@ -440,7 +434,6 @@ func Test_store_SelectLogs(t *testing.T) { }, { "delete covers partial time range", - storeFixture, newQuery( "{foo=~\"ba.*\"}", from, @@ -499,53 +492,12 @@ func Test_store_SelectLogs(t *testing.T) { }, }, }, - { - "rename using label_format", - storeFixtureV4, - newQuery("{foo=\"glop\"} | label_format boo=\"z\" | boo=\"z\"", from, from.Add(6*time.Millisecond), nil, nil), - []logproto.Stream{ - { - Labels: "{boo=\"z\", foo=\"glop\"}", - Entries: []logproto.Entry{ - { - Timestamp: from.Add(2 * time.Millisecond), - Line: "3", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(3 * time.Millisecond), - Line: "4", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - - { - Timestamp: from.Add(4 * time.Millisecond), - Line: "5", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(5 * time.Millisecond), - Line: "6", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - }, - }, - }, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &LokiStore{ - Store: tt.storeFixture, + Store: storeFixture, cfg: Config{ MaxChunkBatchSize: 10, }, diff --git a/pkg/storage/util_test.go b/pkg/storage/util_test.go index bb8a61db33b33..5ef02e74b1caf 100644 --- a/pkg/storage/util_test.go +++ b/pkg/storage/util_test.go @@ -402,77 +402,5 @@ var streamsFixture = []*logproto.Stream{ }, }, }, - { - Labels: "{foo=\"glop\"}", - Entries: []logproto.Entry{ - { - Timestamp: from.Add(2 * time.Millisecond), - Line: "3", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(3 * time.Millisecond), - Line: "4", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - - { - Timestamp: from.Add(4 * time.Millisecond), - Line: "5", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(5 * time.Millisecond), - Line: "6", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - }, - }, } var storeFixture = newMockChunkStore(chunkenc.ChunkFormatV3, chunkenc.UnorderedWithStructuredMetadataHeadBlockFmt, streamsFixture) - -var streamsFixtureV4 = []*logproto.Stream{ - { - Labels: "{foo=\"glop\"}", - Entries: []logproto.Entry{ - { - Timestamp: from.Add(2 * time.Millisecond), - Line: "3", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(3 * time.Millisecond), - Line: "4", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - - { - Timestamp: from.Add(4 * time.Millisecond), - Line: "5", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - { - Timestamp: from.Add(5 * time.Millisecond), - Line: "6", - StructuredMetadata: []logproto.LabelAdapter{ - {Name: "boo", Value: "a"}, - }, - }, - }, - }, -} -var storeFixtureV4 = newMockChunkStore(chunkenc.ChunkFormatV4, chunkenc.UnorderedWithStructuredMetadataHeadBlockFmt, streamsFixtureV4) From 6f97eb97f1ccd41d044690533c6e663e20689e1e Mon Sep 17 00:00:00 2001 From: Ravishankar Date: Thu, 5 Sep 2024 18:39:28 +0530 Subject: [PATCH 3/4] fix: label processing order --- pkg/logql/log/labels.go | 81 +++++++++++++--- pkg/logql/log/labels_test.go | 182 +++++++++++++++++++++++++++++++++++ 2 files changed, 248 insertions(+), 15 deletions(-) diff --git a/pkg/logql/log/labels.go b/pkg/logql/log/labels.go index c68fe1af0e5b5..0739f540ed3e7 100644 --- a/pkg/logql/log/labels.go +++ b/pkg/logql/log/labels.go @@ -327,20 +327,43 @@ func (b *LabelsBuilder) Get(key string) (string, bool) { // Del deletes the label of the given name. func (b *LabelsBuilder) Del(ns ...string) *LabelsBuilder { for _, n := range ns { - for category, lbls := range b.add { - for i, a := range lbls { - if a.Name == n { - b.add[category] = append(lbls[:i], lbls[i+1:]...) - } - } + for category := range b.add { + b.deleteWithCategory(LabelCategory(category), n) } b.del = append(b.del, n) } return b } +// deleteWithCategory removes the label from the specified category +func (b *LabelsBuilder) deleteWithCategory(category LabelCategory, n string) { + for i, l := range b.add[category] { + if l.Name == n { + b.add[category] = append(b.add[category][:i], b.add[category][i+1:]...) + } + } +} + // Set the name/value pair as a label. func (b *LabelsBuilder) Set(category LabelCategory, n, v string) *LabelsBuilder { + if category == ParsedLabel { + b.deleteWithCategory(StructuredMetadataLabel, n) + b.deleteWithCategory(StreamLabel, n) + } + + if category == StructuredMetadataLabel { + b.deleteWithCategory(StreamLabel, n) + if labelsContain(b.add[ParsedLabel], n) { + return b + } + } + + if category == StreamLabel { + if labelsContain(b.add[StructuredMetadataLabel], n) || labelsContain(b.add[ParsedLabel], n) { + return b + } + } + for i, a := range b.add[category] { if a.Name == n { b.add[category][i].Value = v @@ -430,6 +453,7 @@ func (b *LabelsBuilder) UnsortedLabels(buf labels.Labels, categories ...LabelCat } else { buf = buf[:0] } + if categoriesContain(categories, StreamLabel) { Outer: for _, l := range b.base { @@ -439,20 +463,38 @@ func (b *LabelsBuilder) UnsortedLabels(buf labels.Labels, categories ...LabelCat continue Outer } } - // Skip stream labels which value will be replaced - for _, lbls := range b.add { - for _, la := range lbls { - if l.Name == la.Name { - continue Outer - } - } + + // Skip stream labels which value will be replaced by structured metadata + if labelsContain(b.add[StructuredMetadataLabel], l.Name) { + continue + } + + // Skip stream labels which value will be replaced by parsed labels + if labelsContain(b.add[ParsedLabel], l.Name) { + continue + } + + // Take value from stream label if present + if labelsContain(b.add[StreamLabel], l.Name) { + buf = append(buf, labels.Label{Name: l.Name, Value: b.add[StreamLabel].Get(l.Name)}) + } else { + buf = append(buf, l) + } + } + } + + if categoriesContain(categories, StructuredMetadataLabel) { + for _, l := range b.add[StructuredMetadataLabel] { + if labelsContain(b.add[ParsedLabel], l.Name) { + continue } + buf = append(buf, l) } } - for _, category := range categories { - buf = append(buf, b.add[category]...) + if categoriesContain(categories, ParsedLabel) { + buf = append(buf, b.add[ParsedLabel]...) } if (b.HasErr() || b.HasErrorDetails()) && categoriesContain(categories, ParsedLabel) { buf = b.appendErrors(buf) @@ -566,6 +608,15 @@ func flattenLabels(buf labels.Labels, many ...labels.Labels) labels.Labels { return buf } +func labelsContain(labels labels.Labels, name string) bool { + for _, l := range labels { + if l.Name == name { + return true + } + } + return false +} + func (b *BaseLabelsBuilder) toUncategorizedResult(buf labels.Labels) LabelsResult { hash := b.hasher.Hash(buf) if cached, ok := b.resultCache[hash]; ok { diff --git a/pkg/logql/log/labels_test.go b/pkg/logql/log/labels_test.go index 97c9a8899c223..3d3aed705e43e 100644 --- a/pkg/logql/log/labels_test.go +++ b/pkg/logql/log/labels_test.go @@ -1,6 +1,7 @@ package log import ( + "sort" "testing" "github.com/prometheus/prometheus/model/labels" @@ -198,6 +199,187 @@ func TestLabelsBuilder_LabelsResult(t *testing.T) { assert.Equal(t, expectedStreamLbls, actual.Stream()) assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) assert.Equal(t, expectedParsedLbls, actual.Parsed()) + + b.Reset() + b.Set(StreamLabel, "namespace", "tempo") + b.Set(StreamLabel, "bazz", "tazz") + b.Set(StructuredMetadataLabel, "bazz", "sazz") + b.Set(ParsedLabel, "ToReplace", "other") + + expectedStreamLbls = labels.FromStrings( + "namespace", "tempo", + "cluster", "us-central1", + "job", "us-central1/loki", + ) + expectedStucturedMetadataLbls = labels.FromStrings( + "bazz", "sazz", + ) + expectedParsedLbls = labels.FromStrings( + "ToReplace", "other", + ) + + expected = make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + assertLabelResult(t, expected, b.LabelsResult()) + // cached. + assertLabelResult(t, expected, b.LabelsResult()) + actual = b.LabelsResult() + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) +} + +func TestLabelsBuilder_Set(t *testing.T) { + strs := []string{ + "namespace", "loki", + "cluster", "us-central1", + "toreplace", "fuzz", + } + lbs := labels.FromStrings(strs...) + b := NewBaseLabelsBuilder().ForLabels(lbs, lbs.Hash()) + + // test duplicating stream label with parsed label + b.Set(StructuredMetadataLabel, "stzz", "stvzz") + b.Set(ParsedLabel, "toreplace", "buzz") + expectedStreamLbls := labels.FromStrings("namespace", "loki", "cluster", "us-central1") + expectedStucturedMetadataLbls := labels.FromStrings("stzz", "stvzz") + expectedParsedLbls := labels.FromStrings("toreplace", "buzz") + + expected := make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + + actual := b.LabelsResult() + assertLabelResult(t, expected, actual) + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) + + b.Reset() + + // test duplicating structured metadata label with parsed label + b.Set(StructuredMetadataLabel, "stzz", "stvzz") + b.Set(StructuredMetadataLabel, "toreplace", "muzz") + b.Set(ParsedLabel, "toreplace", "buzz") + expectedStreamLbls = labels.FromStrings("namespace", "loki", "cluster", "us-central1") + expectedStucturedMetadataLbls = labels.FromStrings("stzz", "stvzz") + expectedParsedLbls = labels.FromStrings("toreplace", "buzz") + + expected = make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + + actual = b.LabelsResult() + assertLabelResult(t, expected, actual) + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) + + b.Reset() + + // test duplicating stream label with structured meta data label + b.Set(StructuredMetadataLabel, "toreplace", "muzz") + b.Set(ParsedLabel, "stzz", "stvzz") + expectedStreamLbls = labels.FromStrings("namespace", "loki", "cluster", "us-central1") + expectedStucturedMetadataLbls = labels.FromStrings("toreplace", "muzz") + expectedParsedLbls = labels.FromStrings("stzz", "stvzz") + + expected = make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + + actual = b.LabelsResult() + assertLabelResult(t, expected, actual) + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) + + b.Reset() + + // test duplicating parsed label with structured meta data label + b.Set(ParsedLabel, "toreplace", "puzz") + b.Set(StructuredMetadataLabel, "stzz", "stvzzz") + b.Set(StructuredMetadataLabel, "toreplace", "muzz") + expectedStreamLbls = labels.FromStrings("namespace", "loki", "cluster", "us-central1") + expectedStucturedMetadataLbls = labels.FromStrings("stzz", "stvzzz") + expectedParsedLbls = labels.FromStrings("toreplace", "puzz") + + expected = make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + + actual = b.LabelsResult() + assertLabelResult(t, expected, actual) + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) + + b.Reset() + + // test duplicating structured meta data label with stream label + b.Set(ParsedLabel, "stzz", "stvzzz") + b.Set(StructuredMetadataLabel, "toreplace", "muzz") + expectedStreamLbls = labels.FromStrings("namespace", "loki", "cluster", "us-central1") + expectedStucturedMetadataLbls = labels.FromStrings("toreplace", "muzz") + expectedParsedLbls = labels.FromStrings("stzz", "stvzzz") + + expected = make(labels.Labels, 0, len(expectedStreamLbls)+len(expectedStucturedMetadataLbls)+len(expectedParsedLbls)) + expected = append(expected, expectedStreamLbls...) + expected = append(expected, expectedStucturedMetadataLbls...) + expected = append(expected, expectedParsedLbls...) + expected = labels.New(expected...) + + actual = b.LabelsResult() + assertLabelResult(t, expected, actual) + assert.Equal(t, expectedStreamLbls, actual.Stream()) + assert.Equal(t, expectedStucturedMetadataLbls, actual.StructuredMetadata()) + assert.Equal(t, expectedParsedLbls, actual.Parsed()) +} + +func TestLabelsBuilder_UnsortedLabels(t *testing.T) { + strs := []string{ + "namespace", "loki", + "cluster", "us-central1", + "toreplace", "fuzz", + } + lbs := labels.FromStrings(strs...) + b := NewBaseLabelsBuilder().ForLabels(lbs, lbs.Hash()) + b.add[StructuredMetadataLabel] = labels.FromStrings("toreplace", "buzz", "fzz", "bzz") + b.add[ParsedLabel] = labels.FromStrings("pzz", "pvzz") + expected := labels.FromStrings("cluster", "us-central1", "namespace", "loki", "fzz", "bzz", "toreplace", "buzz", "pzz", "pvzz") + actual := b.UnsortedLabels(nil) + sort.Sort(expected) + sort.Sort(actual) + assert.Equal(t, expected, actual) + + b.Reset() + b.add[StructuredMetadataLabel] = labels.FromStrings("fzz", "bzz") + b.add[ParsedLabel] = labels.FromStrings("toreplace", "buzz", "pzz", "pvzz") + expected = labels.FromStrings("cluster", "us-central1", "namespace", "loki", "fzz", "bzz", "toreplace", "buzz", "pzz", "pvzz") + actual = b.UnsortedLabels(nil) + sort.Sort(expected) + sort.Sort(actual) + assert.Equal(t, expected, actual) + + b.Reset() + b.add[StructuredMetadataLabel] = labels.FromStrings("fzz", "bzz", "toreplacezz", "test") + b.add[ParsedLabel] = labels.FromStrings("toreplacezz", "buzz", "pzz", "pvzz") + expected = labels.FromStrings("cluster", "us-central1", "namespace", "loki", "fzz", "bzz", "toreplace", "fuzz", "pzz", "pvzz", "toreplacezz", "buzz") + actual = b.UnsortedLabels(nil) + sort.Sort(expected) + sort.Sort(actual) + assert.Equal(t, expected, actual) } func TestLabelsBuilder_GroupedLabelsResult(t *testing.T) { From 2630b563b3c37c53dc669b0f6619f5317bf031e0 Mon Sep 17 00:00:00 2001 From: Ravishankar Date: Thu, 26 Sep 2024 17:59:38 +0530 Subject: [PATCH 4/4] Review comments --- pkg/logql/log/labels.go | 9 +++++++++ pkg/logql/log/labels_test.go | 4 +--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/logql/log/labels.go b/pkg/logql/log/labels.go index 0739f540ed3e7..c8d0bcb31ebb4 100644 --- a/pkg/logql/log/labels.go +++ b/pkg/logql/log/labels.go @@ -345,12 +345,19 @@ func (b *LabelsBuilder) deleteWithCategory(category LabelCategory, n string) { } // Set the name/value pair as a label. +// The value `v` may not be set if a category with higher preference already contains `n`. +// Category preference goes as Parsed > Structured Metadata > Stream. func (b *LabelsBuilder) Set(category LabelCategory, n, v string) *LabelsBuilder { + // Parsed takes precedence over Structured Metadata and Stream labels. + // If category is Parsed, we delete `n` from the structured metadata and stream labels. if category == ParsedLabel { b.deleteWithCategory(StructuredMetadataLabel, n) b.deleteWithCategory(StreamLabel, n) } + // Structured Metadata takes precedence over Stream labels. + // If category is `StructuredMetadataLabel`,we delete `n` from the stream labels. + // If `n` exists in the parsed labels, we won't overwrite it's value and we just return what we have. if category == StructuredMetadataLabel { b.deleteWithCategory(StreamLabel, n) if labelsContain(b.add[ParsedLabel], n) { @@ -358,6 +365,8 @@ func (b *LabelsBuilder) Set(category LabelCategory, n, v string) *LabelsBuilder } } + // Finally, if category is `StreamLabel` and `n` already exists in either the structured metadata or + // parsed labels, the `Set` operation is a noop and we return the unmodified labels builder. if category == StreamLabel { if labelsContain(b.add[StructuredMetadataLabel], n) || labelsContain(b.add[ParsedLabel], n) { return b diff --git a/pkg/logql/log/labels_test.go b/pkg/logql/log/labels_test.go index 3d3aed705e43e..7f543a48d7d82 100644 --- a/pkg/logql/log/labels_test.go +++ b/pkg/logql/log/labels_test.go @@ -359,9 +359,7 @@ func TestLabelsBuilder_UnsortedLabels(t *testing.T) { b.add[ParsedLabel] = labels.FromStrings("pzz", "pvzz") expected := labels.FromStrings("cluster", "us-central1", "namespace", "loki", "fzz", "bzz", "toreplace", "buzz", "pzz", "pvzz") actual := b.UnsortedLabels(nil) - sort.Sort(expected) - sort.Sort(actual) - assert.Equal(t, expected, actual) + require.ElementsMatch(t, expected, actual) b.Reset() b.add[StructuredMetadataLabel] = labels.FromStrings("fzz", "bzz")