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

[ML] Single Metric Viewer embeddable: Adds action for dashboard to apply filter from the embeddable to the page #198869

Merged
merged 17 commits into from
Nov 18, 2024
Merged

[ML] Single Metric Viewer embeddable: Adds action for dashboard to apply filter from the embeddable to the page #198869

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f396d2a
add filter icons for value and negate value to smv embeddable
alvarezmelissa87 Nov 1, 2024
2400d61
update copy and ensure anomaly charts are consistent
alvarezmelissa87 Nov 6, 2024
e806f0f
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 6, 2024
4cf0dd6
update snapshot
alvarezmelissa87 Nov 6, 2024
9b52eef
remove parens and truncate for charts in dashboard
alvarezmelissa87 Nov 7, 2024
41c37ae
update types
alvarezmelissa87 Nov 8, 2024
b50ea4f
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 11, 2024
21cb885
fix anomaly charts entity layout
alvarezmelissa87 Nov 11, 2024
c6a3be9
update jest test
alvarezmelissa87 Nov 11, 2024
163bc58
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 12, 2024
eaf3776
leave anomaly charts label as they were
alvarezmelissa87 Nov 12, 2024
877fe30
ensure anomaly charts badge layout is unchanged
alvarezmelissa87 Nov 12, 2024
8a4ccbf
rename flag and remove unnecessary one from top level
alvarezmelissa87 Nov 14, 2024
2e6a09c
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 14, 2024
ec42d20
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 14, 2024
8a293e1
[CI] Auto-commit changed files from 'make api-docs'
kibanamachine Nov 14, 2024
805877f
Merge branch 'main' into ml-smv-updates
elasticmachine Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 38 additions & 18 deletions ...er_charts/components/explorer_chart_label/__snapshots__/explorer_chart_label.test.js.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

108 changes: 57 additions & 51 deletions .../explorer/explorer_charts/components/explorer_chart_label/entity_filter/entity_filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import type { FC } from 'react';
import React from 'react';
import { EuiButtonIcon, EuiToolTip } from '@elastic/eui';
import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem, EuiToolTip } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n-react';
import { i18n } from '@kbn/i18n';
import type { MlEntityFieldOperation } from '@kbn/ml-anomaly-utils';
Expand All @@ -22,64 +22,70 @@ interface EntityFilterProps {
}) => void;
influencerFieldName: string;
influencerFieldValue: string;
mode?: string;
}
export const EntityFilter: FC<EntityFilterProps> = ({
onFilter,
influencerFieldName,
influencerFieldValue,
mode,
}) => {
return (
<React.Fragment>
<EuiToolTip
content={
<FormattedMessage
id="xpack.ml.entityFilter.addFilterTooltip"
defaultMessage="Add filter"
<EuiFlexGroup gutterSize="none" alignItems="center">
<EuiFlexItem grow={false}>
<EuiToolTip
content={
<FormattedMessage
id="xpack.ml.entityFilter.addFilterTooltip"
defaultMessage="Filter for"
/>
}
>
<EuiButtonIcon
size="s"
className="filter-button"
onClick={blurButtonOnClick(() => {
onFilter({
influencerFieldName,
influencerFieldValue,
action: ML_ENTITY_FIELD_OPERATIONS.ADD,
});
})}
iconType="plusInCircle"
aria-label={i18n.translate('xpack.ml.entityFilter.addFilterAriaLabel', {
defaultMessage: 'Add filter for {influencerFieldName} {influencerFieldValue}',
values: { influencerFieldName, influencerFieldValue },
})}
/>
}
>
<EuiButtonIcon
size="s"
className="filter-button"
onClick={blurButtonOnClick(() => {
onFilter({
influencerFieldName,
influencerFieldValue,
action: ML_ENTITY_FIELD_OPERATIONS.ADD,
});
})}
iconType="plusInCircle"
aria-label={i18n.translate('xpack.ml.entityFilter.addFilterAriaLabel', {
defaultMessage: 'Add filter for {influencerFieldName} {influencerFieldValue}',
values: { influencerFieldName, influencerFieldValue },
})}
/>
</EuiToolTip>
<EuiToolTip
content={
<FormattedMessage
id="xpack.ml.entityFilter.removeFilterTooltip"
defaultMessage="Remove filter"
</EuiToolTip>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiToolTip
content={
<FormattedMessage
id="xpack.ml.entityFilter.removeFilterTooltip"
defaultMessage={mode ? 'Filter out' : 'Remove filter'}
Copy link
Member

Choose a reason for hiding this comment

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

This will need two different translations. There can't be two possible values for one translation ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - updated in 41c37ae

/>
}
>
<EuiButtonIcon
size="s"
className="filter-button"
onClick={blurButtonOnClick(() => {
onFilter({
influencerFieldName,
influencerFieldValue,
action: ML_ENTITY_FIELD_OPERATIONS.REMOVE,
});
})}
iconType="minusInCircle"
aria-label={i18n.translate('xpack.ml.entityFilter.removeFilterAriaLabel', {
defaultMessage: 'Remove filter for {influencerFieldName} {influencerFieldValue}',
values: { influencerFieldName, influencerFieldValue },
})}
/>
}
>
<EuiButtonIcon
size="s"
className="filter-button"
onClick={blurButtonOnClick(() => {
onFilter({
influencerFieldName,
influencerFieldValue,
action: ML_ENTITY_FIELD_OPERATIONS.REMOVE,
});
})}
iconType="minusInCircle"
aria-label={i18n.translate('xpack.ml.entityFilter.removeFilterAriaLabel', {
defaultMessage: 'Remove filter for {influencerFieldName} {influencerFieldValue}',
values: { influencerFieldName, influencerFieldValue },
})}
/>
</EuiToolTip>
</React.Fragment>
</EuiToolTip>
</EuiFlexItem>
</EuiFlexGroup>
);
};
35 changes: 25 additions & 10 deletions ...lication/explorer/explorer_charts/components/explorer_chart_label/explorer_chart_label.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@

import './_explorer_chart_label.scss';
import PropTypes from 'prop-types';
import React, { Fragment, useCallback } from 'react';
import React, { useCallback } from 'react';

import { EuiIconTip } from '@elastic/eui';
import { EuiFlexGroup, EuiFlexItem, EuiIconTip, EuiText, EuiTextColor } from '@elastic/eui';

import { ExplorerChartLabelBadge } from './explorer_chart_label_badge';
import { ExplorerChartInfoTooltip } from '../../explorer_chart_info_tooltip';
Expand All @@ -19,6 +19,7 @@ export function ExplorerChartLabel({
detectorLabel,
entityFields,
infoTooltip,
mode,
wrapLabel = false,
onSelectEntity,
}) {
Expand Down Expand Up @@ -48,16 +49,30 @@ export function ExplorerChartLabel({
const entityFieldBadges = entityFields.map((entity) => {
const key = `${infoTooltip.chartFunction}-${entity.fieldName}-${entity.fieldType}-${entity.fieldValue}`;
return (
<Fragment key={`badge-wrapper-${key}`}>
<ExplorerChartLabelBadge entity={entity} />
<EuiFlexGroup gutterSize="none" alignItems="center" key={`badge-wrapper-${key}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes here causing the badges in the Anomaly Explorer charts to be rendered with line breaks after the detector description label and before the info tooltip?

Before:
Screenshot 2024-11-11 at 10 29 34

After:
Screenshot 2024-11-11 at 10 16 45

Can the line breaks be removed? (Could also remove the - character before the badge - not sure that adds anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I went ahead and reverted most of the changes to this layout since it relies on some css and made the least amount of changes possible. 21cb885
We should update this to use components in a separate PR.

<EuiFlexItem grow={false}>
{mode === 'embeddable' ? (
<EuiText size="xs">
<EuiTextColor color={'success'} component={'span'}>
{`(${entity.fieldName}: ${entity.fieldValue})`}
</EuiTextColor>
</EuiText>
) : (
<ExplorerChartLabelBadge entity={entity} />
)}
</EuiFlexItem>

{onSelectEntity !== undefined && (
<EntityFilter
onFilter={applyFilter}
influencerFieldName={entity.fieldName}
influencerFieldValue={entity.fieldValue}
/>
<EuiFlexItem grow={false}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably isn't a new issue, but for long field names / values, the filter icons can end up being rendered out of the visible area:

Screenshot 2024-11-07 at 13 14 06

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs a separate issue - happens on main and also affects the charts when in the Anomaly Explorer.

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was able to improve it in 9b52eef for dashboards with truncating and adding in a tooltip for the full entity value:
image

Note that the badges in the explorer view will still overlap. We should create a separate issue for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this needs some more tweaking. The anomaly charts look good for long values, but not for short text values there can be a large gap:

Screenshot 2024-11-08 at 10 16 14

Copy link
Contributor

Choose a reason for hiding this comment

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

Latest changes look good for shorter field name / values. Separate issue created to improve for long values: #199549

<EntityFilter
mode={mode}
onFilter={applyFilter}
influencerFieldName={entity.fieldName}
influencerFieldValue={entity.fieldValue}
/>
</EuiFlexItem>
)}
</Fragment>
</EuiFlexGroup>
);
});

Expand Down
3 changes: 3 additions & 0 deletions ...k/plugins/ml/public/application/explorer/explorer_charts/explorer_anomalies_container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import type { AnomaliesTableData } from '../explorer_utils';

interface ExplorerAnomaliesContainerProps {
id: string;
mode?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Can this have union of explicit values?

Copy link
Member

Choose a reason for hiding this comment

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

Also, it looks like this is always supplied, so doesn't need to be optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, higher up in the component chain, it will either not be provided or be "embeddable" - but by this point it will always be provided. Updated in 41c37ae

chartsData: ExplorerChartsData;
showCharts: boolean;
severity: TableSeverity;
Expand Down Expand Up @@ -51,6 +52,7 @@ const tooManyBucketsCalloutMsg = i18n.translate(

export const ExplorerAnomaliesContainer: FC<ExplorerAnomaliesContainerProps> = ({
id,
mode,
chartsData,
showCharts,
severity,
Expand Down Expand Up @@ -90,6 +92,7 @@ export const ExplorerAnomaliesContainer: FC<ExplorerAnomaliesContainerProps> = (
<ExplorerChartsContainer
{...{
...chartsData,
mode,
severity: severity.val,
mlLocator,
tableData,
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_charts_container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ function getChartId(series, randomId) {
// Wrapper for a single explorer chart
function ExplorerChartContainer({
id,
mode,
series,
severity,
tooManyBuckets,
Expand Down Expand Up @@ -251,6 +252,7 @@ function ExplorerChartContainer({
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<ExplorerChartLabel
mode={mode}
detectorLabel={DetectorLabel}
entityFields={entityFields}
infoTooltip={{ ...series.infoTooltip, chartType }}
Expand Down Expand Up @@ -376,6 +378,7 @@ function ExplorerChartContainer({
// Flex layout wrapper for all explorer charts
export const ExplorerChartsContainerUI = ({
id: uuid,
mode,
chartsPerRow,
seriesToPlot,
severity,
Expand Down Expand Up @@ -443,6 +446,7 @@ export const ExplorerChartsContainerUI = ({
<ExplorerChartContainer
key={chartId}
id={chartId}
mode={mode}
series={series}
severity={severity}
tooManyBuckets={tooManyBuckets}
Expand Down
Loading