-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Changes from 5 commits
d0fbf3c
a83da68
8120941
bab6ca0
a50c635
40c5953
b915271
b24112a
6568360
2f9b01e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -342,7 +342,24 @@ 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) | ||
|
||
// NOTE(kavi): whether the `label` exists in logline matters when doing `!=` or `!~`. | ||
// Because, consider following long lines | ||
// ``` | ||
// 1.msg="hello" subqueries=5 | ||
// 2.msg="hello" subqueries=0 | ||
// 3.msg="hello" | ||
// 4.msg="hello" subqueries="" | ||
//``` | ||
|
||
// Now when quering like `{...}|logfmt|subqueries=""` should consider lines 3 and 4. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this, it doesn't really make it easier to understand for the end user as this make it more obscure:
Honestly in #6713 I think the expectations were wrong.
When subqueries doesn't exist then subqueries!="0" is true. What is weird was that empty labels does transform into a 0 integer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So you saying
That's because, we don't mutate the log line if label is not present. So treat it as if label="" during There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree with @cyriltovena. This is the part I think we need to address. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes that's possible. We can skip empty values during metrics extraction when using unwrap without changing any of the existing label filter behavior. Also the more I think about it, subqueries!="0" and subqueries!=0 are two different label filters and should be treated as such. And former can indeed match log lines that doesn't have subqueries label at all (we are assuming it has default empty string substring=" when doesn't exist). I have change the PR like it's mentioned here. |
||
// But when doing queries like `{...}|logfmt|subqueries!="0"` (also with !~), then line 3 shouldn't be considered. | ||
// https://github.com/grafana/loki/pull/6766 | ||
// https://github.com/grafana/loki/issues/6713 | ||
if !ok && (s.Type == labels.MatchNotEqual || s.Type == labels.MatchNotRegexp) { | ||
return line, false | ||
} | ||
return line, s.Matches(v) | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.