Logs: Add toggle behavior support for "filter for" and "filter out" label within Logs Details #70091
Conversation
|
Backend code coverage report for PR #70091 |
|
Frontend code coverage report for PR #70091
|
|
i like this new behaviour. it took me a while to see what the different was compared current implementation, but as you mention in the pr, once the button colour changes to indicate this is a toggle i think it will work great and there should be any confusion there. |
matyax
force-pushed
the
matyax/log-details-toggle-off
branch
from
a0d0b0e
to
c2a9514
Compare
matyax
changed the title
matyax
force-pushed
the
matyax/log-details-toggle-off
branch
from
0cb58b5
to
109a742
Compare
| expression = queryHasFilter(expression, action.options.key, '=', value) | ||
| ? removeLabelFromQuery(expression, action.options.key, '=', value) | ||
| : addLabelToQuery(expression, action.options.key, '=', value); |
There was a problem hiding this comment.
I am not sure if we should align names of those methods in the future. Currently "Filter" is more like the LabelFilter, which isn't a stream-selector, I would say. But as said, we may do this in the future and not necessarily in this PR.
| let selector: SyntaxNode | null = matcher; | ||
| do { | ||
| selector = selector.parent; | ||
| } while (selector && selector.type.id !== Selector); |
There was a problem hiding this comment.
Would make another great util function, or addition to NodePosition, to get a parent by type.
There was a problem hiding this comment.
Absolutely. It didn't exist when I worked in the module, but will consider adding it in a follow up PR.
| let expression = query.query ?? ''; | ||
| switch (action.type) { | ||
| case 'ADD_FILTER': { | ||
| if (expression.length > 0) { | ||
| expression += ' AND '; | ||
| } | ||
| expression += `${action.options.key}:"${action.options.value}"`; | ||
| // This gives the user the ability to toggle a filter on and off. | ||
| expression = queryHasFilter(expression, action.options.key, action.options.value) | ||
| ? removeFilterFromQuery(expression, action.options.key, action.options.value) | ||
| : addFilterToQuery(expression, action.options.key, action.options.value); | ||
| break; | ||
| } | ||
| case 'ADD_FILTER_OUT': { | ||
| if (expression.length > 0) { | ||
| expression += ' AND '; | ||
| /** | ||
| * If there is a filter with the same key and value, remove it. | ||
| * This prevents the user from seeing no changes in the query when they apply | ||
| * this filter. | ||
| */ | ||
| if (queryHasFilter(expression, action.options.key, action.options.value)) { | ||
| expression = removeFilterFromQuery(expression, action.options.key, action.options.value); | ||
| } | ||
| expression += `-${action.options.key}:"${action.options.value}"`; | ||
| expression = addFilterToQuery(expression, action.options.key, action.options.value, '-'); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not sure if we can easily solve this for ES without a language/lucene parser. E.g. this will break queries like (hostname:"hostname4" OR hostname:"hostname3") AND (label:"val3" OR label:"val4").
I know that the previous implementation of this in ES wasn't great, but at least it did not end in "broken" queries. WDYT about skipping ES for now and come back to this once we have a lucene parser? also @gabor WDYT?
There was a problem hiding this comment.
I agree. I think we could feature flag it until we implement the parser, which will follow up this.
There was a problem hiding this comment.
Unless you or @gabor disagree, I would follow this one up with adding the feature flag in another PR.
|
Thank you! Will follow up with the feature flag soon. |
…abel within Logs Details (#70091) * Datasource test: fix describe nesting * Parsing: export handleQuotes function * Modify query: add functions to detect the presence of a label and remove it * Loki: add support to toggle filters if already present * Datasource test: fix describe nesting * Loki: add support to toggle filter out if present * Remove label: handle escaped values * Datasource: add test case for escaped label values * Loki: remove = filter when applying != * Remove selector: add support for Selector node being far from Matcher * Modify query: add unit tests * Elasticsearch: create modifyQuery for elastic * Elastic modify query: implement functions * Elasticsearch: implement modifyQuery functions in datasource * Elasticsearch: update datasource test * Loki modify query: check for streamSelectorPositions length * Elasticsearch query has filter: escape filter value in regex * Remove unused type * Modify query: use query modeller instance from module
…abel within Logs Details (#70091) * Datasource test: fix describe nesting * Parsing: export handleQuotes function * Modify query: add functions to detect the presence of a label and remove it * Loki: add support to toggle filters if already present * Datasource test: fix describe nesting * Loki: add support to toggle filter out if present * Remove label: handle escaped values * Datasource: add test case for escaped label values * Loki: remove = filter when applying != * Remove selector: add support for Selector node being far from Matcher * Modify query: add unit tests * Elasticsearch: create modifyQuery for elastic * Elastic modify query: implement functions * Elasticsearch: implement modifyQuery functions in datasource * Elasticsearch: update datasource test * Loki modify query: check for streamSelectorPositions length * Elasticsearch query has filter: escape filter value in regex * Remove unused type * Modify query: use query modeller instance from module
This is the first part of the changes related with Easier search and filter.
What changed
Now, the
button has a toggle behavior. That means, if the filter is already present in the current query, it will be removed from it.
Pending change: this feature will work best when we display the active status of present filters in the query by changing the icon color
. Will be done in a follow-up PR.
Now the
is smarter. When some label needs to be excluded from the query, but it previously existed as a positive filter, we remove it and add the new filtered out key, value pair.
How it used to work
Before these changes, the users would interact with the 🔍 icons with no change in the current query.
Support
These changes are currently supported by both Loki and Elasticsearch, with and without using mixed datasources.
What's next
Demo in Elasticsearch
Elastic.mov
Demo with mixed datasources
Mixed.mov
Which issue(s) does this PR fix?:
Part of #61765
Special notes for your reviewer:
Test the changes, see how it feels after them, let me know if anything is broken or could work in a different way. In particular, the query modifiers for Elasticsearch, they need to be battle tested.