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.

Sign up for GitHub

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

Jump to bottom

[Cloud Security] Improve graph component performance #204983

Merged
merged 6 commits into from
Dec 24, 2024
Merged

[Cloud Security] Improve graph component performance #204983

merged 6 commits into from
Dec 24, 2024

Conversation

kfirpeled
Copy link
Contributor

@kfirpeled kfirpeled commented Dec 19, 2024 •
edited by kibanamachine
Loading

Summary

Apply some graph optimizations and added the first graph benchmark storybook.

You can read the detailed investigation process here.

I kept the dashed lines until a new design will be introduced.

Before:

Screen.Recording.2024-12-23.at.14.04.26.mov

After:

Untitled.mov

How to test this PR

To test this PR you can run

yarn storybook cloud_security_posture_packages

Or open the storybook link attached to the build message

Checklist

@kfirpeled kfirpeled added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 19, 2024
@kfirpeled kfirpeled linked an issue Dec 19, 2024 that may be closed by this pull request
Investigate the graph's performance issues #204982
Closed
1 task
@kfirpeled kfirpeled force-pushed the cspm/graph-comp-perf-imprv branch from d995dbb to 6f02b77 Compare December 23, 2024 13:03
@kfirpeled kfirpeled marked this pull request as ready for review December 23, 2024 13:46
@kfirpeled kfirpeled requested a review from a team as a code owner December 23, 2024 13:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@kfirpeled kfirpeled changed the title [WIP] [Cloud Security] Improve graph component performance [Cloud Security] Improve graph component performance Dec 23, 2024
@kfirpeled
Added graph benchmark with a naive fps gauge
f1950ad 
- Avoiding sub-pixel rendering
- Toggled `onlyRenderVisibleElements` to improve performance
@kfirpeled kfirpeled force-pushed the cspm/graph-comp-perf-imprv branch from 41543eb to f1950ad Compare December 23, 2024 15:01
JordanSh
JordanSh approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@JordanSh JordanSh left a comment

Choose a reason for hiding this comment

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

CR only.
Looks good and memoizing this component makes a lot of sense considering how much is going on. I'm not too familiar with the entire component so I mainly added comments about improving readability for now.

...ions/security/packages/kbn-cloud-security-posture/graph/src/components/edge/default_edge.tsx Outdated Show resolved Hide resolved
...ions/security/packages/kbn-cloud-security-posture/graph/src/components/edge/default_edge.tsx Outdated
const sourceMargin = getShapeHandlePosition(data?.sourceShape);
const targetMargin = getShapeHandlePosition(data?.targetShape);
const markerStart =
data?.sourceShape !== 'label' && data?.sourceShape !== 'group'
Copy link
Contributor

Choose a reason for hiding this comment

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

consider exporting constants for core values like label and group here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a convenient name for sharing constants' literals that are used in schema. Do you have an example? (both sourceShape and targetShape are well defined types)

I tried using enum, but that causes more duplications of typings than good.

Copy link
Contributor

Choose a reason for hiding this comment

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

SHAPE_LABEL, SHAPE_GROUP or an object that can be used like SHAPES.LABEL, SHAPES.GROUP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried it I placed the constants in the /types and used them in the /schema definition. However, the types are defined by schema as well, so it creates cyclic references and when I split it into multiple files and tried to work with it, it didn't recognize it as the same type in some scenarios.

can you try it yourself? I'm probably missing something

...ions/security/packages/kbn-cloud-security-posture/graph/src/components/edge/default_edge.tsx Outdated Show resolved Hide resolved
...ions/security/packages/kbn-cloud-security-posture/graph/src/components/edge/default_edge.tsx Outdated Show resolved Hide resolved
.../solutions/security/packages/kbn-cloud-security-posture/graph/src/components/graph/graph.tsx
Comment on lines +174 to +175
maxZoom={1.3}
minZoom={0.1}
Copy link
Contributor

Choose a reason for hiding this comment

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

not important right now but in general there are a lot of settings scattered around which might be better to export as consts from one file or object, to make it easier to configure in the future if needed

albertoblaz
albertoblaz approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@albertoblaz albertoblaz left a comment

Choose a reason for hiding this comment

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

Same as @JordanSh, I'm not too familiar with this feature, but mostly I added comments about readability and simplicity. Feel free to ignore 👍

@kfirpeled kfirpeled enabled auto-merge (squash) December 24, 2024 17:31
@kfirpeled kfirpeled merged commit d5fe929 into elastic:main Dec 24, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12485249572

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6507 6508 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/cloud-security-posture-graph 19 18 -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 21.4MB 21.4MB +297.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/cloud-security-posture-graph 16 21 +5

Total ESLint disabled count

id before after diff
@kbn/cloud-security-posture-graph 18 23 +5

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 24, 2024
@kfirpeled
[Cloud Security] Improve graph component performance (elastic#204983)
d4c759e 
## Summary

Apply some graph optimizations and added the first graph benchmark
storybook.

You can read the detailed investigation process
[here](elastic#204982 (comment)).

I kept the dashed lines until a new design will be introduced.

Before:

https://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b

After:

https://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9

**How to test this PR**

To test this PR you can run

```
yarn storybook cloud_security_posture_packages
```

Or open the storybook link attached to the build message

### Checklist

- [ ] [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

(cherry picked from commit d5fe929)
@kibanamachine kibanamachine mentioned this pull request Dec 24, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

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 24, 2024
@kibanamachine @kfirpeled
[8.x] [Cloud Security] Improve graph component performance (#204983) (#…
91b6a29 
…205148)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Cloud Security] Improve graph component performance
(#204983)](#204983)

<!--- 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-24T18:52:26Z","message":"[Cloud
Security] Improve graph component performance (#204983)\n\n##
Summary\r\n\r\nApply some graph optimizations and added the first graph
benchmark\r\nstorybook.\r\n\r\nYou can read the detailed investigation
process\r\n[here](https://github.com/elastic/kibana/issues/204982#issuecomment-2559715178).\r\n\r\nI
kept the dashed lines until a new design will be
introduced.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b\r\n\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9\r\n\r\n\r\n**How
to test this PR**\r\n\r\nTo test this PR you can run\r\n\r\n```\r\nyarn
storybook cloud_security_posture_packages\r\n```\r\n\r\nOr open the
storybook link attached to the build message\r\n\r\n###
Checklist\r\n\r\n- [ ] [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","sha":"d5fe9290e80a1b7ac05861804e0fa0e42f05572d","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Cloud
Security","backport:prev-minor","ci:build-storybooks"],"title":"[Cloud
Security] Improve graph component
performance","number":204983,"url":"https://github.com/elastic/kibana/pull/204983","mergeCommit":{"message":"[Cloud
Security] Improve graph component performance (#204983)\n\n##
Summary\r\n\r\nApply some graph optimizations and added the first graph
benchmark\r\nstorybook.\r\n\r\nYou can read the detailed investigation
process\r\n[here](https://github.com/elastic/kibana/issues/204982#issuecomment-2559715178).\r\n\r\nI
kept the dashed lines until a new design will be
introduced.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b\r\n\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9\r\n\r\n\r\n**How
to test this PR**\r\n\r\nTo test this PR you can run\r\n\r\n```\r\nyarn
storybook cloud_security_posture_packages\r\n```\r\n\r\nOr open the
storybook link attached to the build message\r\n\r\n###
Checklist\r\n\r\n- [ ] [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","sha":"d5fe9290e80a1b7ac05861804e0fa0e42f05572d"}},"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/204983","number":204983,"mergeCommit":{"message":"[Cloud
Security] Improve graph component performance (#204983)\n\n##
Summary\r\n\r\nApply some graph optimizations and added the first graph
benchmark\r\nstorybook.\r\n\r\nYou can read the detailed investigation
process\r\n[here](https://github.com/elastic/kibana/issues/204982#issuecomment-2559715178).\r\n\r\nI
kept the dashed lines until a new design will be
introduced.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b\r\n\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9\r\n\r\n\r\n**How
to test this PR**\r\n\r\nTo test this PR you can run\r\n\r\n```\r\nyarn
storybook cloud_security_posture_packages\r\n```\r\n\r\nOr open the
storybook link attached to the build message\r\n\r\n###
Checklist\r\n\r\n- [ ] [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","sha":"d5fe9290e80a1b7ac05861804e0fa0e42f05572d"}}]}]
BACKPORT-->

Co-authored-by: Kfir Peled <[email protected]>
stratoula pushed a commit to stratoula/kibana that referenced this pull request Jan 2, 2025
@kfirpeled @stratoula
[Cloud Security] Improve graph component performance (elastic#204983)
98f7ed0 
## Summary

Apply some graph optimizations and added the first graph benchmark
storybook.

You can read the detailed investigation process
[here](elastic#204982 (comment)).

I kept the dashed lines until a new design will be introduced.

Before:


https://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b


After:


https://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9


**How to test this PR**

To test this PR you can run

```
yarn storybook cloud_security_posture_packages
```

Or open the storybook link attached to the build message

### Checklist

- [ ] [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
benakansara pushed a commit to benakansara/kibana that referenced this pull request Jan 2, 2025
@kfirpeled @benakansara
[Cloud Security] Improve graph component performance (elastic#204983)
84c855f 
## Summary

Apply some graph optimizations and added the first graph benchmark
storybook.

You can read the detailed investigation process
[here](elastic#204982 (comment)).

I kept the dashed lines until a new design will be introduced.

Before:


https://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b


After:


https://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9


**How to test this PR**

To test this PR you can run

```
yarn storybook cloud_security_posture_packages
```

Or open the storybook link attached to the build message

### Checklist

- [ ] [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
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
@kfirpeled @CAWilson94
[Cloud Security] Improve graph component performance (elastic#204983)
0eea012 
## Summary

Apply some graph optimizations and added the first graph benchmark
storybook.

You can read the detailed investigation process
[here](elastic#204982 (comment)).

I kept the dashed lines until a new design will be introduced.

Before:


https://github.com/user-attachments/assets/145a82c0-dbd0-410f-a451-eaa8adcc931b


After:


https://github.com/user-attachments/assets/3c13cae5-85d2-481b-8fc6-cec08ecee0d9


**How to test this PR**

To test this PR you can run

```
yarn storybook cloud_security_posture_packages
```

Or open the storybook link attached to the build message

### Checklist

- [ ] [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JordanSh JordanSh JordanSh approved these changes

@albertoblaz albertoblaz albertoblaz approved these changes

@maxcold maxcold Awaiting requested review from maxcold

Assignees
No one assigned
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:build-storybooks release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate the graph's performance issues
5 participants
@kfirpeled @elasticmachine @kibanamachine @albertoblaz @JordanSh