Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(logql): Make LabelSampleExtractor work as expected when label doesn't exist in logline #6766

Merged
merged 10 commits into from
Aug 3, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `StringLabelfilter` work as expected

##### Changes

Expand Down
5 changes: 4 additions & 1 deletion pkg/logql/log/label_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,10 @@ 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)
v, ok := lbs.Get(s.Name)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the real fix.

if !ok {
return line, false
}
return line, s.Matches(v)
}

Expand Down
30 changes: 30 additions & 0 deletions pkg/logql/log/label_filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -350,3 +351,32 @@ func TestReduceAndLabelFilter(t *testing.T) {
})
}
}

func TestStringLabelFilter(t *testing.T) {
tests := []struct {
name string
filter *StringLabelFilter
labels labels.Labels
shouldMatch bool
}{
{
name: "without-label",
filter: NewStringLabelFilter(labels.MustNewMatcher(labels.MatchNotEqual, "subqueries", "0")),
labels: labels.Labels{{Name: "msg", Value: "hello"}}, // no label `subqueries`
shouldMatch: false,
},
kavirajk marked this conversation as resolved.
Show resolved Hide resolved
{
name: "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,
},
}

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)
})
}
}
64 changes: 64 additions & 0 deletions pkg/logql/log/metrics_extraction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

"github.com/prometheus/prometheus/model/labels"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -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),
ssncferreira marked this conversation as resolved.
Show resolved Hide resolved
),
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 {
Expand Down