-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
Loki: Filter by labels based on the type of label (structured, indexed, parsed) #78595
Loki: Filter by labels based on the type of label (structured, indexed, parsed) #78595
Conversation
(Open the links below in a new tab to go to the correct steps)
|
I took a look at the levitate breaking changes and they are fine. We are working on improving these cases where comments or optional new attributes create these false positives. |
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.
Left some comments/questions bellow. But overall really great job. The test coverage is :chefs-kiss:
@@ -264,6 +264,7 @@ export interface QueryFilterOptions extends KeyValue<string> {} | |||
export interface ToggleFilterAction { | |||
type: 'FILTER_FOR' | 'FILTER_OUT'; |
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.
Not related to this PR but just a note - would be nice if we could type type: 'FILTER_FOR' | 'FILTER_OUT';
using QueryFixType
somehow.
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.
Hm, not sure about a cleaner way here. @matyax maybe?
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.
Maybe for 11 we can clean up all these types, and even rename some to reflect, for example, if you're adding a "field" (or label) filter or a string filter.
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.
Looks really good! Left some questions and comments.
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.
LGTM! I added a suggestion to make it more clear why is the logic to add filter when labelType
is not set much more complex than when it is - because at that point, the query might be metric and we need to add label to each expression. Cause it took me a second to realize that.
What is this feature?
This feature improves the filter actions in the LogDetails for Loki. When the dataframe contains a definition about the label type, indexed labels will always be added to the stream selector. Parsed labels and keys from structured metadata will be added as LabelFilter expressions.
Screen.Recording.2023-11-23.at.14.33.16.mov
How to test
lokiStructuredMetadata
feature toggle nothing should change.lokiStructuredMetadata
set, labels should be added accordingly.