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

Update correlations overview page and flyout #1248

Merged
merged 19 commits into from
Jan 31, 2025

Conversation

vikhy-aws
Copy link
Contributor

@vikhy-aws vikhy-aws commented Jan 28, 2025

Description

This PR adds a new table and a bar graph that displays information by aggregating data on correlation rule, adds a toggle button to toggle visualization between the existing network graph and the newly created table. Also adds a new flyout which describes more information about correlated findings.

Screenshots of pages updated

image image image

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Collaborator

@AWSHurneyt AWSHurneyt left a comment

Choose a reason for hiding this comment

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

Could we add unit tests (or take a fast follow-up item to add unit tests)? Here's an example test
https://github.com/opensearch-project/security-analytics-dashboards-plugin/blob/main/public/pages/Overview/utils/helper.test.ts

We should also take an action item to add cypress (integration) tests.

const findingsMap = new Map<string, CorrelationFinding>();

correlations.forEach((correlation) => {
const { finding1, finding2 } = correlation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@riysaxen-amzn could you clarify whether there are any circumstances when a finding for a correlation could be null? For example, if a finding history index is deleted, will there be any risk in findings being null?

@vikhy-aws
Copy link
Contributor Author

vikhy-aws commented Jan 30, 2025

Updating the graph to be force directed. Graph too cluttered when there are a lot of nodes.
image

@vikhy-aws
Copy link
Contributor Author

Changed the graph to be force-directed.

@vikhy-aws
Copy link
Contributor Author

Cypress tests run locally
image
image
image
image

@amsiglan amsiglan merged commit f63b2ca into opensearch-project:main Jan 31, 2025
5 of 8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 31, 2025
* feat: add correlations overview page with a chart and table, and new flyout for the table

Signed-off-by: vikhy-aws <[email protected]>

* fix: remove log statements

Signed-off-by: vikhy-aws <[email protected]>

* fix: update changes to original component

Signed-off-by: vikhy-aws <[email protected]>

* feat: add resources column to the table

Signed-off-by: vikhy-aws <[email protected]>

* fix: maintain different state for graph in the flyout than the main graph

Signed-off-by: vikhy-aws <[email protected]>

* fix: display mitre tactics as a list and add tooltip for the field

Signed-off-by: vikhy-aws <[email protected]>

* fix: update as per comments on PR

Signed-off-by: vikhy-aws <[email protected]>

* fix: update as per comments on PR

Signed-off-by: vikhy-aws <[email protected]>

* fix: move utility function into helpers

Signed-off-by: vikhy-aws <[email protected]>

* fix: make table as default view, add spinner, and alert name in flyout

Signed-off-by: vikhy-aws <[email protected]>

* fix: add a different state variable to track updates to table

Signed-off-by: vikhy-aws <[email protected]>

* fix: add a different state variable to track updates to table

* fix: fix a bug that loading spinner is on when there is no data

Signed-off-by: vikhy-aws <[email protected]>

* fix: fix a bug that loading spinner is on when there is no data

Signed-off-by: vikhy-aws <[email protected]>

* fix: change graph configurations to make it force-directed

Signed-off-by: vikhy-aws <[email protected]>

* fix: change graph configurations to make it force-directed

Signed-off-by: vikhy-aws <[email protected]>

* fix: increase correlations graph panel height

Signed-off-by: vikhy-aws <[email protected]>

* seperate out table-view into a different file

Signed-off-by: vikhy-aws <[email protected]>

* update according to comments on PR

Signed-off-by: vikhy-aws <[email protected]>

---------

Signed-off-by: vikhy-aws <[email protected]>
(cherry picked from commit f63b2ca)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 31, 2025
* feat: add correlations overview page with a chart and table, and new flyout for the table

Signed-off-by: vikhy-aws <[email protected]>

* fix: remove log statements

Signed-off-by: vikhy-aws <[email protected]>

* fix: update changes to original component

Signed-off-by: vikhy-aws <[email protected]>

* feat: add resources column to the table

Signed-off-by: vikhy-aws <[email protected]>

* fix: maintain different state for graph in the flyout than the main graph

Signed-off-by: vikhy-aws <[email protected]>

* fix: display mitre tactics as a list and add tooltip for the field

Signed-off-by: vikhy-aws <[email protected]>

* fix: update as per comments on PR

Signed-off-by: vikhy-aws <[email protected]>

* fix: update as per comments on PR

Signed-off-by: vikhy-aws <[email protected]>

* fix: move utility function into helpers

Signed-off-by: vikhy-aws <[email protected]>

* fix: make table as default view, add spinner, and alert name in flyout

Signed-off-by: vikhy-aws <[email protected]>

* fix: add a different state variable to track updates to table

Signed-off-by: vikhy-aws <[email protected]>

* fix: add a different state variable to track updates to table

* fix: fix a bug that loading spinner is on when there is no data

Signed-off-by: vikhy-aws <[email protected]>

* fix: fix a bug that loading spinner is on when there is no data

Signed-off-by: vikhy-aws <[email protected]>

* fix: change graph configurations to make it force-directed

Signed-off-by: vikhy-aws <[email protected]>

* fix: change graph configurations to make it force-directed

Signed-off-by: vikhy-aws <[email protected]>

* fix: increase correlations graph panel height

Signed-off-by: vikhy-aws <[email protected]>

* seperate out table-view into a different file

Signed-off-by: vikhy-aws <[email protected]>

* update according to comments on PR

Signed-off-by: vikhy-aws <[email protected]>

---------

Signed-off-by: vikhy-aws <[email protected]>
(cherry picked from commit f63b2ca)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants