-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
Log Rows: Added popover menu with filter options when a log line is selected #75306
Conversation
Backend code coverage report for PR #75306 |
Frontend code coverage report for PR #75306
|
44cb409
to
0092ae4
Compare
761c06d
to
7dc0323
Compare
Demo updated. |
8b17841
to
9a66d77
Compare
9a66d77
to
c73d9a9
Compare
This won't work in safari due to #74135 |
@grafana/observability-logs This is ready for review ✨ |
close(); | ||
}} | ||
/> | ||
{onClickFilterValue && ( |
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.
I think we may need a better API to check what types of filters the given data source supports. At the moment we have a single check in https://github.com/grafana/grafana/blob/main/public/app/features/explore/Logs/LogsContainer.tsx#L91 but a datasource may support only label/tag filter (ADD_FILTER / ADD_FILTER_OUT
) but not line filters (ADD_LINE_FILTER / ADD_LINE_FILTER_OUT
) which may lead to showing no-op items in the menu.
We did a very similar thing with Supplementary Queries. A data source implements getSupportedSupplementaryQueryTypes()
providing list of supported types.
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.
I think at some point we should deprecate checking for modifyQuery
in favor of hasToggleableQueryFiltersSupport()
which is the desired behavior (and API) for log details filters.
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.
Are you making more changing around this in this PR? I think at the moment the popover will show for data sources that may not support ADD_LINE_FILTER
but they support ADD_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.
You're right @ifrost, thanks for the reminder! I'll take another look at this.
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.
I will follow up this PR with a fix for this. Something similar to getSupportedSupplementaryQueryTypes()
would make an interesting approach, as I don't think we need a new interface for this.
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.
Will be addressed by #78322
Really cool feature 🙌 Have you considered extending popover menu with Grafana UI Extensions? |
@ifrost thanks for the feedback!
I've no reference of Grafana UI Extensions. What is it? |
Check docs at https://grafana.com/developers/plugin-tools/ui-extensions/. We have one extension point in Explore https://github.com/grafana/grafana/blob/v10.2.0/packages/grafana-data/src/types/pluginExtensions.ts#L119-L123 - it allows plugins and other parts of Grafana to hook into "Add +" button in Explore toolbar. I was thinking that if there was an extension point we could use it to allow creating correlations with defined regex transformation 🤯 Anyway, just wanted to mention it. Extenstion UI is not needed for this PR, just something to think about in the future. |
Gotcha! It's definitely a great idea to "explore" 😉 |
f2889a9
to
8c69abf
Compare
* Loki modifyQuery: add line does not contain query modification * Elastic modifyQuery: implement line filters * Modify query: change action name to not be loki specific * Prettier * Prettier * Elastic: escape filter values
@svennergr FYI I merged all the sub-PRs into this one. |
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, just one nit: sometimes the menu does not disappear after left-clicks on the selected text.
Screen.Recording.2023-11-15.at.15.49.24.mov
Had to cheery-pick the commits from the feature flag and tracking because they were missing in the other PR, probably due to a bad rebase. Will follow up with a PR to address: |
Easier search and filter through the use of a popover menu.
It makes use of the preexisting Explore methods to add filters from Log Details, adding this function to the situation where the user highlights part of a log line.
Hghlights:
Which issue(s) does this PR fix?:
Part of #76129
Related PRs:
Demos
Elasticsearch:
Elastic.demo.mov
Loki:
Loki.demo.mov