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

[Security Solution] Insight filter builder form as markdown plugin #150363

Merged

Conversation

kqualters-elastic
Copy link
Contributor

@kqualters-elastic kqualters-elastic commented Feb 6, 2023

Summary

This pr expands upon the work done in #145240 to make use of the filters builder form from unified_search to serialize filters into a markdown compatible string, so that investigation guides, timeline notes or any other place where text is parsed as markdown can make use of standard kibana filters and view a count of the matching documents at a glance, and open the entire set in timeline as well. These are generally converted to timeline data providers to enable drag and drop query building, however this is not supported for filters of range type, so regular kibana filters are used in that case for now.

Screenshot 2023-02-06 at 3 46 15 PM
Screenshot 2023-02-06 at 3 49 46 PM
Screenshot 2023-02-06 at 3 50 15 PM
Screenshot 2023-02-06 at 3 50 54 PM
Screenshot 2023-02-06 at 3 51 16 PM
Screenshot 2023-02-06 at 3 53 48 PM
Screenshot 2023-02-06 at 3 54 30 PM

Checklist

@kqualters-elastic kqualters-elastic added release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.7.0 Feature:Filters labels Feb 6, 2023
@kqualters-elastic kqualters-elastic changed the title [Draft] [Security Solution] Insight filter builder form as markdown plugin [Security Solution] Insight filter builder form as markdown plugin Feb 6, 2023
@kqualters-elastic kqualters-elastic marked this pull request as ready for review February 6, 2023 20:55
@kqualters-elastic kqualters-elastic requested review from a team as code owners February 6, 2023 20:55
@PhilippeOberti
Copy link
Contributor

@kqualters-elastic I feel from a usability perspective, when editing a rule and trying to use the Add investigation query feature, the modal is so narrow that the values in the dropdown aren't fully displayed.
As you can see in the image below, not much is visible (only a handful of characters)

Screenshot 2023-02-06 at 4 19 45 PM

I know we have a tooltip on the dropdown values but it takes a couple of seconds to appear and which is annoying...

I wouldn't think this would prevent this PR for being merged, but something to keep in mind and improve later?

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

Couldn't test the functionality fully as I'm not familiar enough with what insights really does in investigation guide, but what I saw looked good (except my comment on the width of the modal).

@kqualters-elastic
Copy link
Contributor Author

@PhilippeOberti yes I noticed that as well, the problem is that modals by default have a max-width(768px, calc(100vw - 6px)) imposed by default in Eui https://elastic.github.io/eui/#/layout/modal . To override that, you can either pass a maxWidth: number or maxWidth: false prop, but unfortunately the modal that renders markdown plugins is defined exclusively in Eui, and does not expose a way to pass that prop (or any) through: https://github.com/elastic/eui/blob/main/src/components/markdown_editor/markdown_editor.tsx#L506

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3586 3587 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.9MB 12.9MB +30.0KB
unifiedSearch 264.2KB 264.1KB -69.0B
total +29.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
unifiedSearch 33.2KB 33.5KB +259.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@PhilippeOberti
Copy link
Contributor

Yeah I saw that after I posted my comment. I guess we could have that form row spread over 2 rows to get more space? Again no big deal for this PR it can be improved later!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Unified search changes LGTM. I wonder if it is needed to pass the saved serialized filters from migrations. (as is done in cases for Lens serialized state). If a change happens in the filters SOs shouldnt this be depicted in the saved filters? I am just asking as I am not sure how this works.

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Explore code LGTM!

@@ -445,6 +445,7 @@ export const useSourcererDataView = (
selectedPatterns,
// if we have to do an update to data view, tell us which patterns are active
...(legacyPatterns.length > 0 ? { activePatterns: sourcererDataView.patternList } : {}),
sourcererDataView,
Copy link
Member

Choose a reason for hiding this comment

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

Could we delete some redundant properties of this object? Like dataViewId , browserFields and runtimeMappings ?

@@ -83,7 +83,7 @@ export const useHoverActions = ({
const values = useMemo(() => {
const val = dataProvider.queryMatch.value;

if (typeof val === 'number') {
if (typeof val === 'number' || typeof val === 'boolean') {
Copy link
Member

Choose a reason for hiding this comment

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

FYI: We are planning to delete this file on 8.8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, i was just following the compiler ha

@kqualters-elastic kqualters-elastic merged commit 13d1f39 into elastic:main Feb 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 7, 2023
@kqualters-elastic
Copy link
Contributor Author

@stratoula ya agree completely, more robust handling of this across versions of both the rules package and kibana is the last remaining thing to come in 8.8 and will be when the feature is GA, during technical preview the plan has been to just not throw and break the rest of the app, and make additive only changes to the markdown structure.

@kqualters-elastic kqualters-elastic deleted the insight-filter-builder branch February 8, 2023 01:29
@stratoula
Copy link
Contributor

@kqualters-elastic yes makes sense, thanx! I just wanted to point this out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Filters release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants