diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa022e68222..0b1e8998ea35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ * [6395](https://github.com/grafana/loki/pull/6395) **DylanGuedes**: Add encoding support ##### Fixes +* [6766](https://github.com/grafana/loki/pull/6766) **kavirajk**: fix(logql): Make `LabelSampleExtractor` ignore processing the line if it doesn't contain that specific label. Fixes unwrap behavior explained in the issue https://github.com/grafana/loki/issues/6713 ##### Changes diff --git a/pkg/logql/log/label_filter.go b/pkg/logql/log/label_filter.go index 00871d44d156..b0a86baeb206 100644 --- a/pkg/logql/log/label_filter.go +++ b/pkg/logql/log/label_filter.go @@ -342,6 +342,7 @@ func (s *StringLabelFilter) Process(_ int64, line []byte, lbs *LabelsBuilder) ([ if s.Name == logqlmodel.ErrorLabel { return line, s.Matches(lbs.GetErr()) } + v, _ := lbs.Get(s.Name) return line, s.Matches(v) } diff --git a/pkg/logql/log/label_filter_test.go b/pkg/logql/log/label_filter_test.go index 2bc3c232d408..e9c8a70a8f26 100644 --- a/pkg/logql/log/label_filter_test.go +++ b/pkg/logql/log/label_filter_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/prometheus/prometheus/model/labels" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/loki/pkg/logqlmodel" @@ -350,3 +351,72 @@ func TestReduceAndLabelFilter(t *testing.T) { }) } } + +func TestStringLabelFilter(t *testing.T) { + // NOTE: https://github.com/grafana/loki/issues/6713 + + tests := []struct { + name string + filter *StringLabelFilter + labels labels.Labels + shouldMatch bool + }{ + { + name: `logfmt|subqueries!="0" (without label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotEqual, "subqueries", "0")), + labels: labels.Labels{{Name: "msg", Value: "hello"}}, // no label `subqueries` + // without `subqueries` label, the value is assumed to be empty `subqueries=""` is matches the label filter `subqueries!="0"`. + shouldMatch: true, + }, + { + name: `logfmt|subqueries!="0" (with label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotEqual, "subqueries", "0")), + labels: labels.Labels{{Name: "msg", Value: "hello"}, {Name: "subqueries", Value: "2"}}, // label `subqueries` exist + shouldMatch: true, + }, + { + name: `logfmt|subqueries!~"0" (without label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotRegexp, "subqueries", "0")), + labels: labels.Labels{{Name: "msg", Value: "hello"}}, // no label `subqueries` + // without `subqueries` label, the value is assumed to be empty `subqueries=""` is matches the label filter `subqueries!="0"`. + shouldMatch: true, + }, + { + name: `logfmt|subqueries!~"0" (with label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotRegexp, "subqueries", "0")), + labels: labels.Labels{{Name: "msg", Value: "hello"}, {Name: "subqueries", Value: "2"}}, // label `subqueries` exist + shouldMatch: true, + }, + { + name: `logfmt|subqueries="0" (without label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchEqual, "subqueries", "")), + labels: labels.Labels{{Name: "msg", Value: "hello"}}, // no label `subqueries` + shouldMatch: true, + }, + { + name: `logfmt|subqueries="0" (with label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchEqual, "subqueries", "")), + labels: labels.Labels{{Name: "msg", Value: "hello"}, {Name: "subqueries", Value: ""}}, // label `subqueries` exist + shouldMatch: true, + }, + { + name: `logfmt|subqueries=~"0" (without label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchRegexp, "subqueries", "")), + labels: labels.Labels{{Name: "msg", Value: "hello"}}, // no label `subqueries` + shouldMatch: true, + }, + { + name: `logfmt|subqueries=~"0" (with label)`, + filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchRegexp, "subqueries", "")), + labels: labels.Labels{{Name: "msg", Value: "hello"}, {Name: "subqueries", Value: ""}}, // label `subqueries` exist + shouldMatch: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, ok := tc.filter.Process(0, []byte("sample log line"), NewBaseLabelsBuilder().ForLabels(tc.labels, tc.labels.Hash())) + assert.Equal(t, tc.shouldMatch, ok) + }) + } +} diff --git a/pkg/logql/log/metrics_extraction.go b/pkg/logql/log/metrics_extraction.go index bebf0791b08a..7b5a0a48d8b3 100644 --- a/pkg/logql/log/metrics_extraction.go +++ b/pkg/logql/log/metrics_extraction.go @@ -179,15 +179,18 @@ func (l *streamLabelSampleExtractor) Process(ts int64, line []byte) (float64, La var v float64 stringValue, _ := l.builder.Get(l.labelName) if stringValue == "" { + // NOTE: It's totally fine for log line to not have this particular label. + // See Issue: https://github.com/grafana/loki/issues/6713 + return 0, nil, false + } + + var err error + v, err = l.conversionFn(stringValue) + if err != nil { l.builder.SetErr(errSampleExtraction) - } else { - var err error - v, err = l.conversionFn(stringValue) - if err != nil { - l.builder.SetErr(errSampleExtraction) - l.builder.SetErrorDetails(err.Error()) - } + l.builder.SetErrorDetails(err.Error()) } + // post filters if _, ok = l.postFilter.Process(ts, line, l.builder); !ok { return 0, nil, false diff --git a/pkg/logql/log/metrics_extraction_test.go b/pkg/logql/log/metrics_extraction_test.go index f6d456051d85..7b3b7400fc1f 100644 --- a/pkg/logql/log/metrics_extraction_test.go +++ b/pkg/logql/log/metrics_extraction_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/prometheus/prometheus/model/labels" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -152,6 +153,69 @@ func Test_Extract_ExpectedLabels(t *testing.T) { require.True(t, ok) require.Equal(t, (20 * time.Millisecond).Seconds(), f) require.Equal(t, labels.Labels{{Name: "foo", Value: "json"}}, lbs.Labels()) + +} +func TestLabelExtractorWithStages(t *testing.T) { + + // A helper type to check if particular logline should be skipped + // during `ProcessLine` or got correct sample value extracted. + type checkLine struct { + logLine string + skip bool + sample float64 + } + + tests := []struct { + name string + extractor SampleExtractor + checkLines []checkLine + shouldFail bool + }{ + { + name: "with just logfmt and stringlabelfilter", + // {foo="bar"} | logfmt | subqueries != "0" (note: "0", a stringlabelfilter) + extractor: mustSampleExtractor( + LabelExtractorWithStages("subqueries", ConvertFloat, []string{"foo"}, false, false, []Stage{NewLogfmtParser(), NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotEqual, "subqueries", "0"))}, NoopStage), + ), + checkLines: []checkLine{ + {logLine: "msg=hello subqueries=5", skip: false, sample: 5}, + {logLine: "msg=hello subqueries=0", skip: true}, + {logLine: "msg=hello ", skip: true}, // log lines doesn't contain the `subqueries` label + }, + }, + { + name: "with just logfmt and numeric labelfilter", + // {foo="bar"} | logfmt | subqueries != 0 (note: "0", a numericLabelFilter) + extractor: mustSampleExtractor( + LabelExtractorWithStages("subqueries", ConvertFloat, []string{"foo"}, false, false, []Stage{NewLogfmtParser(), NewNumericLabelFilter(LabelFilterNotEqual, "subqueries", 0)}, NoopStage), + ), + checkLines: []checkLine{ + {logLine: "msg=hello subqueries=5", skip: false, sample: 5}, + {logLine: "msg=hello subqueries=0", skip: true}, + {logLine: "msg=hello ", skip: true}, // log lines doesn't contain the `subqueries` label + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + for _, line := range tc.checkLines { + v, lbs, ok := tc.extractor.ForStream(labels.Labels{{Name: "bar", Value: "foo"}}).ProcessString(0, line.logLine) + skipped := !ok + assert.Equal(t, line.skip, skipped, "line", line.logLine) + if !skipped { + assert.Equal(t, line.sample, v) + + // lbs shouldn't have __error__ = SampleExtractionError + assert.Empty(t, lbs.Labels()) + return + } + + // if line is skipped, `lbs` will be nil. + assert.Nil(t, lbs, "line", line.logLine) + } + }) + } } func mustSampleExtractor(ex SampleExtractor, err error) SampleExtractor {