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

[ML] Explain Log Rate Spikes: Adds discover link to analysis table #139877

Merged
merged 11 commits into from
Sep 7, 2022
Merged

[ML] Explain Log Rate Spikes: Adds discover link to analysis table #139877

merged 11 commits into from
Sep 7, 2022

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Aug 31, 2022 •
edited
Loading

Summary

Related meta issue: #138117

This PR adds an Actions column to the Spike Analysis Table with an item to link to discover for that key/value pair.

image

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 added release_note:enhancement v8.5.0 Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis labels Aug 31, 2022
@alvarezmelissa87 alvarezmelissa87 self-assigned this Aug 31, 2022
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner August 31, 2022 23:20
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Aug 31, 2022
Closed
9 tasks
@walterra
Copy link
Contributor

walterra commented Sep 1, 2022

Looking at the screenshot, suggest to add a fixed width to the actions column to make it more narrow.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra
Copy link
Contributor

walterra commented Sep 5, 2022

In Anomaly Explorer's anomaly table, we use a different icon for the link to discover, suggest to use the same here:

image

walterra
walterra reviewed Sep 5, 2022
View reviewed changes
x-pack/plugins/aiops/public/components/spike_analysis_table/spike_analysis_table.tsx Outdated
enabled: () => discoverUrlError === undefined,
},
],
width: '60px',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For the other columns we already have a constant with the custom width (NARROW_COLUMN_WIDTH). Even if this is just one other column, suggest to create another constant like ACTION_COLUMN_WIDTH for this.

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 344f7b0

@alvarezmelissa87
Copy link
Contributor Author

@elasticmachine merge upstream

@alvarezmelissa87
Copy link
Contributor Author

This has been updated with suggestions and is ready for a final look
cc @walterra, @peteharverson, @darnautov

darnautov
darnautov approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

Code LGTM, just need to remove one commented line

x-pack/plugins/aiops/public/components/spike_analysis_table/spike_analysis_table.tsx Outdated Show resolved Hide resolved
peteharverson
peteharverson approved these changes Sep 7, 2022
View reviewed changes
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 latest changes and LGTM

@alvarezmelissa87 @darnautov
Update x-pack/plugins/aiops/public/components/spike_analysis_table/sp…
29f25f1 
…ike_analysis_table.tsx

Co-authored-by: Dima Arnautov <arnautov.dima@gmail.com>
@kibana-ci
Copy link
Collaborator

💚 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
aiops 478.1KB 479.6KB +1.5KB

History

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

cc @alvarezmelissa87

@alvarezmelissa87 alvarezmelissa87 merged commit 7a83e87 into elastic:main Sep 7, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 7, 2022
@alvarezmelissa87 alvarezmelissa87 deleted the ml-aiops-discover-link branch September 7, 2022 15:58
@peteharverson peteharverson changed the title [ML] AI Ops: Adds discover link to analysis table [ML] Explain Log Rate Spikes: Adds discover link to analysis table Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterra walterra walterra left review comments

@darnautov darnautov darnautov approved these changes

@peteharverson peteharverson peteharverson approved these changes

Assignees

@alvarezmelissa87 alvarezmelissa87

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.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alvarezmelissa87 @walterra @elasticmachine @kibana-ci @darnautov @peteharverson @kibanamachine