-
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
[ML] AiOps: Action for adding Log Pattern embeddable to a dashboard and case #199478
base: main
Are you sure you want to change the base?
Conversation
const isCasesEmbeddable = embeddingOrigin === AIOPS_EMBEDDABLE_ORIGIN.CASES; | ||
|
||
// Disable filtering for cases embeddable, as it's not possible to delete filters | ||
const actions = [...(isCasesEmbeddable ? [] : getActions(false)), ...getActions(true)]; |
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.
It disables filtering for a pattern in cases. In dashboards, it is possible to delete a filter, but there is currently no such option in cases. I have disabled it for now, but it would be nice to have that option available inside the embeddable.
|
||
return ( | ||
<> | ||
<FieldValidationCallout validationResults={fieldValidationResult} /> | ||
{(loading ?? true) === true ? <LoadingCategorization onCancel={cancelRequest} /> : null} | ||
{(loading ?? true) === true ? ( | ||
<LoadingCategorization onCancel={cancelRequest} disableCancelLoading={isCasesEmbeddable} /> |
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.
Disabled cancel loading
in cases because it is not possible to refresh the embeddable. Therefore, after canceling, the embeddable would get stuck without results.
@@ -101,10 +101,6 @@ export const ChartGridEmbeddableWrapper: FC<ChangePointDetectionProps> = ({ | |||
10000 | |||
); | |||
|
|||
useEffect(() => { |
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.
Pinging @elastic/ml-ui (:ml) |
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.
Great update! I tested adding pattern analysis to dashboards&cases from the page. Added some minor comments:
const SavedObjectSaveModalDashboard = withSuspense(LazySavedObjectSaveModalDashboard); | ||
|
||
interface AttachmentsMenuProps { | ||
dataView: DataView; |
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.
Could this use useAiopsAppContext
for some of the props here?
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.
Done in: #c696e4e
|
||
const onSave: SaveModalDashboardProps['onSave'] = useCallback( | ||
({ dashboardId, newTitle, newDescription }) => { | ||
const stateTransfer = embeddable!.getStateTransfer(); |
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: Is it possible to avoid the !
? If you keep passing in embeddable via props you could make it non-optional, other maybe return early? (I saw it's the same for the change point one so probably fine to leave as is if too much trouble refactoring)
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 embeddable is no longer passed as a prop since we're now calling useAiopsAppContext
directly. In this scenario, I don't think there's much we can do about it. We could return early in the onSave callback, but that’s all.
I tried modifying the AiopsAppContext type to differentiate when embeddable should be defined, but achieving this isn’t that easy.
@@ -20,9 +20,10 @@ import { FormattedMessage } from '@kbn/i18n-react'; | |||
|
|||
interface Props { | |||
onCancel: () => void; | |||
disableCancelLoading?: boolean; |
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: An alternative to avoid another prop could be to make onCancel optional and set the button to disabled when it's not set.
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.
Done in: #d162011
randomSamplerProbability: RandomSamplerProbability; | ||
embeddable?: EmbeddableStart; | ||
cases?: CasesPublicStart; | ||
capabilities?: Capabilities; |
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.
capabilities shouldn't be optional
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.
]); | ||
|
||
return ( | ||
<> |
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, this fragment isn't needed
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.
Done in: #d162011
@@ -20,9 +20,10 @@ import { FormattedMessage } from '@kbn/i18n-react'; | |||
|
|||
interface Props { | |||
onCancel: () => void; | |||
disableCancelLoading?: boolean; |
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.
rather than passing in an additional prop to disable this, onCancel
could be made optional and the button only enabled if onCancel
is defined.
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.
Done in: #d162011
const actions = [...getActions(false), ...getActions(true)]; | ||
const isCasesEmbeddable = embeddingOrigin === AIOPS_EMBEDDABLE_ORIGIN.CASES; | ||
|
||
// Disable filtering for cases embeddable, as it's not possible to delete filters |
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.
// Disable filtering for cases embeddable, as it's not possible to delete filters | |
// When in cases, we can only show the "Filter for pattern in Discover" actions as Cases does not have full filter management. |
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.
Done in: #d162011
const isCasesEmbeddable = embeddingOrigin === AIOPS_EMBEDDABLE_ORIGIN.CASES; | ||
|
||
// Disable filtering for cases embeddable, as it's not possible to delete filters | ||
const actions = [...(isCasesEmbeddable ? [] : getActions(false)), ...getActions(true)]; |
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, I think this is a bit more readable
const actions = [...(isCasesEmbeddable ? [] : getActions(false)), ...getActions(true)]; | |
const actions = isCasesEmbeddable | |
? getActions(true) | |
: [...getActions(false), ...getActions(true)]; |
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.
Done in: #d162011
To slightly increase the bundle size for the PR to pass you need to updated the threshold in this file for |
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.
Added a comment about the cancel button, but otherwise LGTM
|
||
return ( | ||
<> | ||
<FieldValidationCallout validationResults={fieldValidationResult} /> | ||
{(loading ?? true) === true ? <LoadingCategorization onCancel={cancelRequest} /> : null} | ||
{(loading ?? true) === true ? <LoadingCategorization /> : null} |
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 still want the ability to cancel the loading when the embeddable is in a dashboard
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.
Done in: 1c4be00
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.
packages/kbn-optimizer/limits.yml
displayName: i18n.translate('xpack.aiops.logPatternAnalysis.cases.attachmentName', { | ||
defaultMessage: 'Log pattern analysis', | ||
}), | ||
getAttachmentViewObject: () => ({ |
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.
It is optional, but you might also want to include a getAttachmentRemovalObject
here to define a message for when this attachment is deleted from the case.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @rbrtj |
Summary
Part of #197247
Added the ability to add a Log Pattern Embeddable to a dashboard and case.
Fixed the Change Point Detection embeddable in cases and added a functional test to cover this scenario.
Screen.Recording.2024-11-06.at.13.58.01.mov
Checklist
Delete any items that are not applicable to this PR.