[Cloud Security] Show graph visualization in expanded flyout #198240
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kfirpeled
changed the title
kfirpeled
added
ci:build-cloud-image
release_note:skip
kfirpeled
commented
Oct 30, 2024
x-pack/packages/kbn-cloud-security-posture/graph/src/components/node/node_expand_button.tsx
Outdated
Show resolved
Hide resolved
kfirpeled
added
ci:project-redeploy
|
PR Project deployment started at: https://buildkite.com/elastic/kibana-deploy-project-from-pr/builds/124 |
|
Project deployed, see credentials at: https://buildkite.com/elastic/kibana-deploy-project-from-pr/builds/124 |
|
PR Project deployment started at: https://buildkite.com/elastic/kibana-deploy-project-from-pr/builds/125 |
|
Project deployed, see credentials at: https://buildkite.com/elastic/kibana-deploy-project-from-pr/builds/125 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kfirpeled
force-pushed
the
cspm/cdr-graph-viz-expanded
branch
from
a80ecfb to
1f0bcec
Compare
christineweng
approved these changes
Dec 10, 2024
...ecurity_solution/public/flyout/document_details/right/components/graph_preview_container.tsx
Outdated
Show resolved
Hide resolved
| */ | ||
|
|
||
| /** This security solution experimental feature allows user to enable/disable the graph visualization in Flyout feature (depends on securitySolution:enableVisualizationsInFlyout) */ | ||
| export const GRAPH_VISUALIZATION_IN_FLYOUT_ENABLED_EXPERIMENTAL_FEATURE = |
There was a problem hiding this comment.
we've never really had constants like this for experimental feature, but I kinda like it. What do you think @christineweng , is that something we should adopt ourselves as well?
x-pack/plugins/security_solution/public/flyout/document_details/shared/constants/panel_keys.ts
Outdated
Show resolved
Hide resolved
...solution/public/flyout/document_details/shared/hooks/use_navigate_to_graph_visualization.tsx
Outdated
Show resolved
Hide resolved
maxcold
approved these changes
Dec 12, 2024
.../kbn-cloud-security-posture/graph/src/components/graph_investigation/graph_investigation.tsx
Outdated
Show resolved
Hide resolved
| }, | ||
| }, | ||
| options: { | ||
| refetchOnWindowFocus: false, |
There was a problem hiding this comment.
as far as I can see everywhere the useFetchGraphData hook is called, the refetchOnWindowFocus is set to false? Should it be the default value then?
There was a problem hiding this comment.
I kept it false to be consistent with react-query defaults
wdyt?
| }, | ||
| options: { | ||
| refetchOnWindowFocus: false, | ||
| keepPreviousData: true, |
There was a problem hiding this comment.
What is the logic behind different values for keepPreviousData? From the UX perspective I didn't notice any difference when changed to false here with the mocked data? If there is some edge case we want to cover by this option, it might worth a comment
There was a problem hiding this comment.
the logic is that while you query you will see the previous results.
if it is set to false, on every query your graph will be cleaned, and when the response is back it will be drawn. Which creates a flicker.
Besides that, on error you will lose your current graph data. instead of seeing your previous graph data.
I think it provides a better UX
| }, | ||
| }); | ||
|
|
||
| const addFilter = (dataViewId: string, prev: Filter[], key: string, value: string) => { |
There was a problem hiding this comment.
what this logic of combined vs phrase filter is about? not really sure, maybe also worth a comment explaining the edge cases and/or add unit tests wich would explain different scenarious
There was a problem hiding this comment.
so we keep always a single filter with OR statements
So whenever addFilter is called, it checks first if it exists or not, and then adds OR statement when needed
I'll add comments now and UT later, once I'll have graph_investigation in storybook (it requires some extra mocking and this PR is already quite big).
.../kbn-cloud-security-posture/graph/src/components/graph_investigation/graph_investigation.tsx
Outdated
Show resolved
Hide resolved
| : undefined; | ||
| return { ...node, ...nodeHandlers }; | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
any reason not to add expandButtonClickHandler to the deps map?
There was a problem hiding this comment.
yea, it stops behaving as expected :P
(seriously, when I remove the filters it renders an empty graph although the graph api response is not empty)
I honestly don't remember why atm. But we can try to invest more time to understand why it causes issues
.../kbn-cloud-security-posture/graph/src/components/graph_investigation/graph_investigation.tsx
Outdated
Show resolved
Hide resolved
...loud-security-posture/graph/src/components/graph_investigation/graph_node_expand_popover.tsx
Outdated
Show resolved
Hide resolved
| [closePopoverHandler] | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
it feels like it should be possible to get rid of this effect in favour of directly doing all this in the onNodeExpandButtonClick handler. But maybe I'm missing smth
x-pack/plugins/security_solution/public/flyout/document_details/left/tabs/visualize_tab.tsx
Outdated
Show resolved
Hide resolved
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
|
PhilippeOberti
approved these changes
Dec 12, 2024
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 12, 2024
…#198240) ## Summary Added graph tab to the flyout visualization of alerts and events. **A couple of included changes:** - Added technical preview badge - ~Feature is now toggled using `securitySolution:enableVisualizationsInFlyout` advanced setting~ reverted back to use the experimental feature flag - Added node popover to expand the graph - Expanding a graph adds relevant filters - Added e2e tests for both alerts flyout and events flyout (through network page) **List of known issues:** - The graph API works queries `logs-*` while the filters bar works with sourcerer current dataview Id - I'm not sure how to write a UT for GraphVisualization / Popover which uses ReactPortal that makes it tricky to test (I covered most scenarios using E2E test) - Expanding graph more than once adds another filter **How to test this PR:** - Enable the feature flag `kibana.dev.yml`: ```yaml uiSettings.overrides.securitySolution:enableVisualizationsInFlyout: true xpack.securitySolution.enableExperimental: ['graphVisualizationInFlyoutEnabled'] ``` - Load mocked data: ```bash node scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/logs_gcp_audit \ --es-url http://elastic:changeme@localhost:9200 \ --kibana-url http://elastic:changeme@localhost:5601 node scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/security_alerts \ --es-url http://elastic:changeme@localhost:9200 \ --kibana-url http://elastic:changeme@localhost:5601 ``` - Make sure you include data from Oct 13 2024. (in the video I use Last 90 days) https://github.com/user-attachments/assets/12e19ac7-0f61-4c0a-ac11-e304dfcc83d4 ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 749eeec)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Dec 13, 2024
…198240) (#204143) # Backport This will backport the following commits from `main` to `8.x`: - [[Cloud Security] Show graph visualization in expanded flyout (#198240)](#198240) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kfir Peled","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-12T22:14:44Z","message":"[Cloud Security] Show graph visualization in expanded flyout (#198240)\n\n## Summary\r\n\r\nAdded graph tab to the flyout visualization of alerts and events.\r\n\r\n**A couple of included changes:**\r\n- Added technical preview badge\r\n- ~Feature is now toggled using\r\n`securitySolution:enableVisualizationsInFlyout` advanced setting~\r\nreverted back to use the experimental feature flag\r\n- Added node popover to expand the graph\r\n- Expanding a graph adds relevant filters\r\n- Added e2e tests for both alerts flyout and events flyout (through\r\nnetwork page)\r\n\r\n**List of known issues:**\r\n- The graph API works queries `logs-*` while the filters bar works with\r\nsourcerer current dataview Id\r\n- I'm not sure how to write a UT for GraphVisualization / Popover which\r\nuses ReactPortal that makes it tricky to test (I covered most scenarios\r\nusing E2E test)\r\n- Expanding graph more than once adds another filter\r\n\r\n\r\n**How to test this PR:**\r\n\r\n- Enable the feature flag \r\n\r\n`kibana.dev.yml`:\r\n\r\n```yaml\r\nuiSettings.overrides.securitySolution:enableVisualizationsInFlyout: true\r\nxpack.securitySolution.enableExperimental: ['graphVisualizationInFlyoutEnabled']\r\n```\r\n\r\n- Load mocked data:\r\n\r\n```bash\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/logs_gcp_audit \\ \r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/security_alerts \\\r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n```\r\n\r\n- Make sure you include data from Oct 13 2024. (in the video I use Last\r\n90 days)\r\n\r\n\r\nhttps://github.com/user-attachments/assets/12e19ac7-0f61-4c0a-ac11-e304dfcc83d4\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"749eeec4ccd50391c6050a9142c1b4fcfccf56d4","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor"],"title":"[Cloud Security] Show graph visualization in expanded flyout","number":198240,"url":"https://github.com/elastic/kibana/pull/198240","mergeCommit":{"message":"[Cloud Security] Show graph visualization in expanded flyout (#198240)\n\n## Summary\r\n\r\nAdded graph tab to the flyout visualization of alerts and events.\r\n\r\n**A couple of included changes:**\r\n- Added technical preview badge\r\n- ~Feature is now toggled using\r\n`securitySolution:enableVisualizationsInFlyout` advanced setting~\r\nreverted back to use the experimental feature flag\r\n- Added node popover to expand the graph\r\n- Expanding a graph adds relevant filters\r\n- Added e2e tests for both alerts flyout and events flyout (through\r\nnetwork page)\r\n\r\n**List of known issues:**\r\n- The graph API works queries `logs-*` while the filters bar works with\r\nsourcerer current dataview Id\r\n- I'm not sure how to write a UT for GraphVisualization / Popover which\r\nuses ReactPortal that makes it tricky to test (I covered most scenarios\r\nusing E2E test)\r\n- Expanding graph more than once adds another filter\r\n\r\n\r\n**How to test this PR:**\r\n\r\n- Enable the feature flag \r\n\r\n`kibana.dev.yml`:\r\n\r\n```yaml\r\nuiSettings.overrides.securitySolution:enableVisualizationsInFlyout: true\r\nxpack.securitySolution.enableExperimental: ['graphVisualizationInFlyoutEnabled']\r\n```\r\n\r\n- Load mocked data:\r\n\r\n```bash\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/logs_gcp_audit \\ \r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/security_alerts \\\r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n```\r\n\r\n- Make sure you include data from Oct 13 2024. (in the video I use Last\r\n90 days)\r\n\r\n\r\nhttps://github.com/user-attachments/assets/12e19ac7-0f61-4c0a-ac11-e304dfcc83d4\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"749eeec4ccd50391c6050a9142c1b4fcfccf56d4"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198240","number":198240,"mergeCommit":{"message":"[Cloud Security] Show graph visualization in expanded flyout (#198240)\n\n## Summary\r\n\r\nAdded graph tab to the flyout visualization of alerts and events.\r\n\r\n**A couple of included changes:**\r\n- Added technical preview badge\r\n- ~Feature is now toggled using\r\n`securitySolution:enableVisualizationsInFlyout` advanced setting~\r\nreverted back to use the experimental feature flag\r\n- Added node popover to expand the graph\r\n- Expanding a graph adds relevant filters\r\n- Added e2e tests for both alerts flyout and events flyout (through\r\nnetwork page)\r\n\r\n**List of known issues:**\r\n- The graph API works queries `logs-*` while the filters bar works with\r\nsourcerer current dataview Id\r\n- I'm not sure how to write a UT for GraphVisualization / Popover which\r\nuses ReactPortal that makes it tricky to test (I covered most scenarios\r\nusing E2E test)\r\n- Expanding graph more than once adds another filter\r\n\r\n\r\n**How to test this PR:**\r\n\r\n- Enable the feature flag \r\n\r\n`kibana.dev.yml`:\r\n\r\n```yaml\r\nuiSettings.overrides.securitySolution:enableVisualizationsInFlyout: true\r\nxpack.securitySolution.enableExperimental: ['graphVisualizationInFlyoutEnabled']\r\n```\r\n\r\n- Load mocked data:\r\n\r\n```bash\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/logs_gcp_audit \\ \r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n\r\nnode scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/security_alerts \\\r\n --es-url http://elastic:changeme@localhost:9200 \\\r\n --kibana-url http://elastic:changeme@localhost:5601\r\n```\r\n\r\n- Make sure you include data from Oct 13 2024. (in the video I use Last\r\n90 days)\r\n\r\n\r\nhttps://github.com/user-attachments/assets/12e19ac7-0f61-4c0a-ac11-e304dfcc83d4\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"749eeec4ccd50391c6050a9142c1b4fcfccf56d4"}}]}] BACKPORT--> Co-authored-by: Kfir Peled <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added graph tab to the flyout visualization of alerts and events.
A couple of included changes:
Feature is now toggled usingreverted back to use the experimental feature flagsecuritySolution:enableVisualizationsInFlyoutadvanced settingList of known issues:
logs-*while the filters bar works with sourcerer current dataview IdHow to test this PR:
kibana.dev.yml:node scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/logs_gcp_audit \ --es-url http://elastic:changeme@localhost:9200 \ --kibana-url http://elastic:changeme@localhost:5601 node scripts/es_archiver load x-pack/test/cloud_security_posture_functional/es_archives/security_alerts \ --es-url http://elastic:changeme@localhost:9200 \ --kibana-url http://elastic:changeme@localhost:5601Screen.Recording.2024-11-20.at.18.23.42.mov
Checklist