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

[SecuritySolution] Enable chartEmbeddablesEnabled feature flag #150531

Merged
merged 39 commits into from Mar 7, 2023
Merged

[SecuritySolution] Enable chartEmbeddablesEnabled feature flag #150531

merged 39 commits into from Mar 7, 2023

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Feb 8, 2023 •
edited

Summary

Original Issue
#136409

Preview
https://kibana-pr-150531.kb.us-west2.gcp.elastic-cloud.com:9243/
https://p.elstc.co/paste/G+PhWdS0#WEyGBtMD9I4r74WPNIQFvgELZPOp-SZCG3yja1LOuwQ

Migrated charts
#149123

Known issues of the Embeddables after enabling the feature flag:
#136409 (Feature request on Lens’ side & Bugs section)

Charts not supported by Lens Embeddable:
#149592

Checklist

Delete any items that are not applicable to this PR.

@angorayc angorayc added Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Explore ci:cloud-deploy Create or update a Cloud deployment v8.8.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 8, 2023
@angorayc angorayc force-pushed the enable-chart branch 2 times, most recently from 815e639 to 7cfa0ad Compare February 21, 2023 09:18
angorayc
angorayc commented Feb 21, 2023
View reviewed changes
x-pack/plugins/security_solution/cypress/e2e/detection_alerts/changing_alert_status.cy.ts
});
});

it('Updates trend histogram whenever alert status is updated in table', () => {
it('Updates count table whenever alert status is updated in table', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MadameSheema , I changed the test description based on the content below, but I might have misunderstood, could you please check if I took it right?

Copy link
Member

Choose a reason for hiding this comment

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

That should be checked with the person who wrote this test.

angorayc
angorayc commented Feb 21, 2023
View reviewed changes
...ugins/security_solution/public/explore/components/risk_score/risk_details_tab_body/index.tsx
const { data, loading, refetch, inspect, isDeprecated, isModuleEnabled } = useRiskScore({
filterQuery,
onlyLatest: false,
riskEntity,
skip: (!overTimeToggleStatus && !contributorsToggleStatus) || isChartEmbeddablesEnabled,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Top contributor table is still relying on this result when feature flag is on, so I shouldn't have skipped it here

@angorayc angorayc marked this pull request as ready for review February 21, 2023 13:57
@angorayc angorayc requested review from a team as code owners February 21, 2023 13:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@angorayc angorayc added the release_note:skip Skip the PR/issue when compiling release notes label Feb 21, 2023
@angorayc angorayc changed the title enable chartEmbeddablesEnabled [SecuritySolution] Enable chartEmbeddablesEnabled feature flag Feb 21, 2023
@machadoum
Copy link
Member

@angorayc I am Not sure if these are already being tracked, but here is what I found:

Entity analytics page - The donut chart legends have filter in and filter out actions, but this page has no Search bar.
Screenshot 2023-02-22 at 13 59 25

Detection & Response, Entity Analytics - When the donut is clicked, it adds a global filter, but these pages don't have the global search bar
Screenshot 2023-02-22 at 14 18 16

Hosts page - Unique IPs add an empty filter when Src or Dest is clicked.
Screenshot 2023-02-22 at 14 07 09

Explore pages - When a day is clicked, the global time range is updated, but the pages don't change.
Feb-22-2023 14-14-17

@machadoum machadoum self-requested a review February 22, 2023 13:51
@angorayc
Copy link
Contributor Author

angorayc commented Mar 2, 2023 •
edited

Just a couple more nits:

The position of the extra actions should probably be bumped higher or given a bottom padding so it doesn't overlap with the visualizations:

Count table:

image

Trend graph:

image

Good point, thank you! I've adjusted the styles.

  1. Table:

Screenshot 2023-03-02 at 10 50 12

2. Bar chart:

Screenshot 2023-03-02 at 10 49 57

yctercero
yctercero approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Codeowners for security platform LGTM.

Quick question - should something be showing in the trends below screenshot?
Screenshot 2023-03-03 at 2 24 06 PM

@angorayc
Copy link
Contributor Author

angorayc commented Mar 6, 2023

Codeowners for security platform LGTM.

Quick question - should something be showing in the trends below screenshot? Screenshot 2023-03-03 at 2 24 06 PM

Hey @yctercero , thanks for your review. May I confirm that the scenario happened when the alert index just created? If that’s the case, it’s one of the known issues: #150818 . The chart uses the method from useSourcerer to check if alert index exists but there's an issue that it has to do a page refresh to get the correct result when alert index just created. I find the issue is in progress, so once it's done, this should be fixed accordingly. I'll follow it up.

MadameSheema
MadameSheema approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

security-engineering-productivity changes LGTM

@yctercero
Copy link
Contributor

Codeowners for security platform LGTM.
Quick question - should something be showing in the trends below screenshot? Screenshot 2023-03-03 at 2 24 06 PM

Hey @yctercero , thanks for your review. May I confirm that the scenario happened when the alert index just created? If that’s the case, it’s one of the known issues: #150818 . The chart uses the method from useSourcerer to check if alert index exists but there's an issue that it has to do a page refresh to get the correct result when alert index just created. I find the issue is in progress, so once it's done, this should be fixed accordingly. I'll follow it up.

Yea, looks like on refresh, charts appear!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Mar 6, 2023 •
edited

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 15.7MB 15.7MB +2.8KB

Page load bundle

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

id before after diff
securitySolution 53.5KB 53.6KB +72.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

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

@angorayc angorayc merged commit 0d8a1ec into elastic:main Mar 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 7, 2023
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
@angorayc @kibanamachine
[SecuritySolution] Enable chartEmbeddablesEnabled feature flag (elast…
6d8ea38 
…ic#150531)

## Summary

**Original Issue**
elastic#136409

**Preview**
https://kibana-pr-150531.kb.us-west2.gcp.elastic-cloud.com:9243/

https://p.elstc.co/paste/G+PhWdS0#WEyGBtMD9I4r74WPNIQFvgELZPOp-SZCG3yja1LOuwQ

**Migrated charts**
elastic#149123

**Known issues of the Embeddables after enabling the feature flag:** 
elastic#136409 (Feature request on
Lens’ side & Bugs section)

**Charts not supported by Lens Embeddable:**
elastic#149592

### Checklist

Delete any items that are not applicable to this PR.

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

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
machadoum added a commit that referenced this pull request Mar 9, 2023
@machadoum
Add cypress tests for inspect button on explore pages (#151714)
78df9a5 
EPIC: elastic/security-team#5979
Test: `C164772 | Validate that Inspect button works correctly and
details display as per the data view selected after the build upgrade`

## Summary
* Group all inspect button tests into one file to make the test quicker.
* Add entries for missing visualizations and tables

Many cypress tests are broken because I enabled the experimental
feature. They will be fixed by this PR
#150531


- [x] Unkip cypress tests listed
[here](#152359)

### Checklist

- [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
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@angorayc @kibanamachine @bmorelli25
[SecuritySolution] Enable chartEmbeddablesEnabled feature flag (elast…
a53c6d2 
…ic#150531)

## Summary

**Original Issue**
elastic#136409

**Preview**
https://kibana-pr-150531.kb.us-west2.gcp.elastic-cloud.com:9243/

https://p.elstc.co/paste/G+PhWdS0#WEyGBtMD9I4r74WPNIQFvgELZPOp-SZCG3yja1LOuwQ

**Migrated charts**
elastic#149123

**Known issues of the Embeddables after enabling the feature flag:** 
elastic#136409 (Feature request on
Lens’ side & Bugs section)

**Charts not supported by Lens Embeddable:**
elastic#149592

### Checklist

Delete any items that are not applicable to this PR.

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

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@machadoum @bmorelli25
Add cypress tests for inspect button on explore pages (elastic#151714)
2dea7d2 
EPIC: elastic/security-team#5979
Test: `C164772 | Validate that Inspect button works correctly and
details display as per the data view selected after the build upgrade`

## Summary
* Group all inspect button tests into one file to make the test quicker.
* Add entries for missing visualizations and tables

Many cypress tests are broken because I enabled the experimental
feature. They will be fixed by this PR
elastic#150531


- [x] Unkip cypress tests listed
[here](elastic#152359)

### Checklist

- [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
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
@angorayc @kibanamachine @nkhristinin
[SecuritySolution] Enable chartEmbeddablesEnabled feature flag (#150531)
e0fd148 
## Summary

**Original Issue**
#136409

**Preview**
https://kibana-pr-150531.kb.us-west2.gcp.elastic-cloud.com:9243/

https://p.elstc.co/paste/G+PhWdS0#WEyGBtMD9I4r74WPNIQFvgELZPOp-SZCG3yja1LOuwQ

**Migrated charts**
#149123

**Known issues of the Embeddables after enabling the feature flag:** 
#136409 (Feature request on
Lens’ side & Bugs section)

**Charts not supported by Lens Embeddable:**
#149592

### Checklist

Delete any items that are not applicable to this PR.

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

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
@machadoum @nkhristinin
Add cypress tests for inspect button on explore pages (#151714)
b1d2915 
EPIC: elastic/security-team#5979
Test: `C164772 | Validate that Inspect button works correctly and
details display as per the data view selected after the build upgrade`

## Summary
* Group all inspect button tests into one file to make the test quicker.
* Add entries for missing visualizations and tables

Many cypress tests are broken because I enabled the experimental
feature. They will be fixed by this PR
#150531


- [x] Unkip cypress tests listed
[here](#152359)

### Checklist

- [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
@angorayc angorayc mentioned this pull request Mar 27, 2023
Closed
41 tasks
@michaelolo24 michaelolo24 mentioned this pull request Dec 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machadoum machadoum machadoum approved these changes

@yctercero yctercero yctercero approved these changes

@michaelolo24 michaelolo24 michaelolo24 approved these changes

@MadameSheema MadameSheema MadameSheema approved these changes

@christineweng christineweng christineweng approved these changes

@marshallmain marshallmain marshallmain approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment needs_docs release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@angorayc @elasticmachine @machadoum @christineweng @michaelolo24 @yctercero @kibana-ci @MadameSheema @marshallmain @kibanamachine