-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] Add visualization actions #126507
Conversation
cd02303
to
b951e07
Compare
))} | ||
<CasesContext owner={[APP_ID]} userCanCrud={userCanCrud ?? false}> | ||
{statItemsProps.map((mappedStatItemProps) => ( | ||
<StatItemsComponent {...mappedStatItemProps} showInspectButton={false} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: Each child in a list should have a unique "key" prop
... is this not showing here? do we need a key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key prop is included in statItemsProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I just had the question about the keys, not blocking. Great work!
@@ -18,6 +18,7 @@ | |||
"eventLog", | |||
"features", | |||
"inspector", | |||
"lens", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have lens
as an optionalPlugins
also, can we add the dependency for a single place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've removed the optionalPlugins and keep it in requiredPlugins
@@ -30,4 +31,5 @@ export const histogramConfigs: MatrixHistogramConfigs = { | |||
stackByOptions: alertsStackByOptions, | |||
subtitle: undefined, | |||
title: i18n.ALERTS_GRAPH_TITLE, | |||
getLensAttributes: getExternalAlertConfigs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we have a more consistent naming approach? Why the getExternalAlertConfigs
is assigned to the property which seems to be a lens attributes - are we using a sub configuration? Maybe then we should assign here only the part of the config which is used for lens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry for the confusion, getExternalAlertConfigs should be renamed to getExternalAlertLensAttributes as it is for getting Lens Attributes only. I've renamed all the Lens attributes and their folder.
@@ -97,7 +100,7 @@ const HeaderSectionComponent: React.FC<HeaderSectionProps> = ({ | |||
)} | |||
</EuiFlexItem> | |||
|
|||
{id && ( | |||
{id && showInspectButton && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test this with the unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/oblt-deploy |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good from design thanks
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
Please add the dev docs about how to create the lens visualization support to the new security visualization (we will need this later for the new stuff). It could be a follow up PR.
Yup, thank you, added the readme here: |
@elasticmachine merge upstream |
💛 Build succeeded, but was flakyTest FailuresMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @angorayc |
* add chart configs * init hosts chart actions * init network chart actions * add to new case * clean up * clean up * clean up configs * rename configs * rename histogram actions to viz actions component * fix up * add vizType * cypress hosts inspect * add viz actions cypress tests * fix type * stat_items unit test * fix unit tests for alerts by category * fix unit tests * unit tests * unit tests * rename vizType from store to inspectedVizType * move out i18n * unit test * add index filter * clean up configs * unit tests * fix typo * rm unused props * apply cases flyout and modal * rm unused definition * fix typo * rm vizType in reducer * onCloseInspect callback * move viz action component out of header section * rm hard coded dataViewId in configs * update icon and wording * fix unit tests * useRouteSpy * showInspectButton * add aria label * rm type casting * unit test * rm id from filters * use mockCasesContract * add unit tests * styling * update mock * rm visualization actions cypress tests * clean up data-test-subj * disabled inspect button in matrix histogram by default * styling * viz actions only available on hosts / network page * rm kpi * unit tests * unit tests * unit tests * unit tests * kibana dependency * rename * add readme Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Add
vizualisation action
component to bar chart, area chart, and matrix histogram.Known issue:
In Security Solution we can filter index patterns under selected Data View in the Sourcerer, but when open the chart in Lens, it only takes all the index patterns under selected Data view id, to fix this, I append
_index
filter when open the chart in Lens as a temporary solution: #126911 (comment)This inconsistency has also be written down in this issue and assigned to the relevant team: #126911
before:
previous_inspect.mp4
after:
openInLens.mp4
custom_chart_actions.mp4