Skip to content

[Cloud Posture] add findings distribution bar#129639

Merged
orouz merged 2 commits intoelastic:mainfrom
orouz:findings_agg
Apr 17, 2022
Merged

[Cloud Posture] add findings distribution bar#129639
orouz merged 2 commits intoelastic:mainfrom
orouz:findings_agg

Conversation

@orouz
Copy link
Copy Markdown
Contributor

@orouz orouz commented Apr 6, 2022

Summary

this PR adds:

  • aggregation query
  • UI for total passed/failed findings

Screen Shot 2022-04-06 at 20 36 34

@orouz orouz force-pushed the findings_agg branch 5 times, most recently from df716ae to be1569a Compare April 6, 2022 18:54
@orouz orouz marked this pull request as ready for review April 7, 2022 08:07
@orouz orouz requested a review from a team as a code owner April 7, 2022 08:07
@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes v8.3.0 labels Apr 7, 2022
@orouz orouz requested a review from ari-aviran April 11, 2022 08:57
@kfirpeled kfirpeled added the Team:Cloud Security Cloud Security team related label Apr 11, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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.

Looks good overall, some nit comments at the start that you can feel free to ignore. But mainly, I think we should stick to using Eui components when we can unless we decide otherwise, but this decision needs to be taken collectively IMO.

Comment on lines 42 to 66
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mega nit but IMO one-liners don't need space between them, especially since they serve the same purpose of preparing the query and can be looked at as a group.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to top

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit I would put this line with the rest of the one-liners at the top, at 62

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if data is undefined does it still makes sense to put 0 here? i can understand total passed or failed can be undefined if nothing is found by the query and we can assume its 0. but if data is undefined its probably some kind of error isn't it?
Also if I'm not mistaken count does return a 0 if it does not find anything.

Comment on lines 130 to 135
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might be better if the consumer of Counter will handle placement and margins for it

Comment on lines 56 to 58
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like this can throw? maybe add error handling since you already have the hook

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it might, not likely tho. i could add a defensive check for it so the type wouldn't be | undefined

in anyway, errors are already handled with react-query's onError

@orouz orouz force-pushed the findings_agg branch 2 times, most recently from 22b28b2 to 4463b04 Compare April 13, 2022 17:19
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this bit with lastValueFrom was not added by us and i had conflicts in this file after rebasing, which led to minor type changes.

@kfirpeled kfirpeled requested a review from JordanSh April 14, 2022 08:19
Copy link
Copy Markdown
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.

🚢

Comment on lines 125 to 149
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this will change later on once we add data to the new table which will use this distribution bar too. for now it needs to be hidden as it reflects only results of group by none

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cloudSecurityPosture 179 239 +60

Async chunks

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

id before after diff
cloudSecurityPosture 371.5KB 407.1KB +35.6KB

Page load bundle

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

id before after diff
cloudSecurityPosture 4.4KB 4.5KB +116.0B

History

  • 💚 Build #38288 succeeded aeaafc29fc7aed571edb30d6029cd0f28933e95f
  • 💔 Build #38272 failed 9f23152ef071880bf7c4bb4f0fc4aafeea5b6ac8
  • 💚 Build #37994 succeeded 1aee0e31dfdb46a55f9d6c1ad2db66dcfe56346d
  • 💔 Build #37987 failed 4463b04436f2cc9fb9ab93b3344e409db3524a18
  • 💚 Build #37252 succeeded fefa4f946f116f6fbc4544b0273e9b78128ee1cd
  • 💚 Build #37219 succeeded a299ee3db2e79fb3af5007886b7da3d6a58be781

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

cc @orouz

@orouz orouz merged commit 1bfeab7 into elastic:main Apr 17, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Apr 17, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants