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.

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: Link from Explain Log Rate Spikes to Log Pattern Analysis #155121

Merged
merged 14 commits into from
Apr 24, 2023

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Apr 18, 2023

Summary

Adds table actions to Explain Log Rate Spikes to be able to drill down to Log Pattern Analysis.

aiops-link-elrs-to-lpa-0001

Checklist

@walterra walterra force-pushed the ml-153753-link-to-log-pattern-analysis branch from 48a1ee1 to 6fee2b8 Compare April 18, 2023 09:57
@walterra walterra added :ml Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis v8.8.0 labels Apr 18, 2023
@walterra walterra marked this pull request as ready for review April 18, 2023 17:52
@walterra walterra requested a review from a team as a code owner April 18, 2023 17:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra
Copy link
Contributor Author

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and overall looks good.

The fact that the link is opening in the same tab highlights the fact that we don't store the state of the analysis in the URL, or the results of the analysis. This means if you hit 'back' in the browser you have to run the analysis all over again.

I also couldn't find a way of opening the link in a new tab, which would make a workflow of running log pattern analysis on different groups / items easier.

return (
<TableActionButton
dataTestSubjPostfix="LogPatternAnalysis"
iconType="logstashQueue"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not easy choosing a suitable icon for this action! The only other EUI one I thought might be suitable was list.


const mlLocator = useMemo(() => share.url.locators.get('ML_APP_LOCATOR'), [share.url.locators]);

const generateLogPatternAnalysisUrl = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that browsers can generally handle URLs 32k more in length, I guess we don't need to worry too much about how long the URL that can be generated here could be? I got up to 5000 chars for a group of 23 items.

@walterra
Copy link
Contributor Author

@peteharverson The behavior is the same like with the already existing link to Discover. The way it's implemented I don't see a quick fix to open in a new tab or support the back button to retain the analysis, that's something we need to follow up on with additional work.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a couple of comments, but generally LGTM

dataTestSubjPostfix: string;
isDisabled: boolean;
label: string;
message?: string;
Copy link
Member

Choose a reason for hiding this comment

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

nit, maybe tooltipText or something similar would be a more descriptive name for this prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in ab28bf2.


return {
render: (tableItem: SignificantTerm | GroupTableItem) => {
const message = logPatternAnalysisUrlError
Copy link
Member

Choose a reason for hiding this comment

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

logPatternAnalysisUrlError is always undefined, is the logic which uses this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch, I missed updating that. I updated with some error handling inspired by the Discover action in ab28bf2.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 496 499 +3

Async chunks

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

id before after diff
aiops 772.5KB 778.0KB +5.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 395 398 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 475 478 +3
total +5

History

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

cc @walterra

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM.

We should definitely look to store the state of the view in the URL, and even cache the results of the analysis somehow to improve that 'back' button behavior, for a future enhancement.

@walterra walterra merged commit 3e94b43 into elastic:main Apr 24, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 24, 2023
@walterra walterra deleted the ml-153753-link-to-log-pattern-analysis branch April 24, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis :ml release_note:enhancement v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants