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: highlight field pairs unique to groups in expanded row #148601

Merged
merged 5 commits into from
Jan 12, 2023
Merged

[ML] Explain Log Rate Spikes: highlight field pairs unique to groups in expanded row #148601

merged 5 commits into from
Jan 12, 2023

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Jan 9, 2023 •
edited
Loading

Summary

Related meta issue: #146162

For grouped results, indicate in the expanded table row field / value pairs that appear in other groups. In expanded row - adds an asterisk column to highlight unique pairs among groups.

image

@alvarezmelissa87 alvarezmelissa87 added :ml Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis v8.7.0 labels Jan 9, 2023
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner January 9, 2023 22:30
@alvarezmelissa87 alvarezmelissa87 self-assigned this Jan 9, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Jan 9, 2023
Closed
5 tasks
@alvarezmelissa87 alvarezmelissa87 added the release_note:skip Skip the PR/issue when compiling release notes label Jan 9, 2023
@walterra
Copy link
Contributor

Here's a version with some tweaks/suggestions. Looking at the screenshot in the PR description it was a bit hard to make out the asterisk in the table, I'm afraid it could be mistaken as part of the field value as some sort of wildcard. I moved the asterisk to the left, it could be an EuiIconTip with type="asterisk" and the text as a tooltip. The mockup is just a suggestion, not sure if it needs to be a separate column or if we could prefix the field name column cells. I also gave it a try to blur the non-unique rows to de-emphasize them. I'm not sure though if the reason for de-emphasis will be obvious, it might need a note in the description at the top of the table too.

image

@peteharverson
Copy link
Contributor

Looking at the suggestion from @walterra in #148601 (comment), I like the positioning of the asterisk on the left hand side in a separate, narrow column. I don't think the fading of the text for the non-unique rows is needed.

peteharverson
peteharverson reviewed Jan 10, 2023
View reviewed changes
...plugins/aiops/public/components/explain_log_rate_spikes/explain_log_rate_spikes_analysis.tsx Outdated
const groupResultsHelpMessage = i18n.translate(
'xpack.aiops.spikeAnalysisTable.groupedSwitchLabel.groupResultsHelpMessage',
{
defaultMessage: "In expanded row, unique group pairs marked by an asterisk ('*')",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest a slight edit here to use the same sort of terminology as used in the badge in the main row:

In expanded row, field/value pairs which do not appear in other groups are marked by an asterisk (*).

Can this message be moved inside the expanded row?

image

@alvarezmelissa87
Copy link
Contributor Author

All suggestions added - this is ready for a final look when you get a chance 🙏 cc @walterra, @peteharverson

peteharverson
peteharverson approved these changes Jan 11, 2023
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 and other than one suggestion on the help text line wrapping, LGTM

...plugins/aiops/public/components/explain_log_rate_spikes/explain_log_rate_spikes_analysis.tsx
</EuiFlexItem>
<EuiFlexItem>
{groupResults && (
<EuiFormRow helpText={groupResultsHelpMessage}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to set fullWidth={true} on the EuiFormRow so that the help message doesn't need to wrap onto a second line.

@peteharverson peteharverson removed the request for review from qn895 January 11, 2023 12:19
@peteharverson peteharverson added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2023
@alvarezmelissa87
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ml-agg-utils 58 59 +1

Async chunks

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

id before after diff
aiops 739.1KB 740.0KB +940.0B
Unknown metric groups

API count

id before after diff
@kbn/ml-agg-utils 82 83 +1

History

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

cc @alvarezmelissa87

walterra
walterra approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM 🥳

@alvarezmelissa87 alvarezmelissa87 merged commit 16d6965 into elastic:main Jan 12, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 12, 2023
@alvarezmelissa87 alvarezmelissa87 deleted the ml-aiops-unique-group-highlight branch January 12, 2023 18:07
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Jan 13, 2023
@alvarezmelissa87 @kibanamachine @jennypavlova
[ML] AIOps: highlight pairs unique to groups in expanded row (elastic…
73023af 
…#148601)

## Summary

Related meta issue: elastic#146162

For grouped results, indicate in the expanded table row field / value
pairs that appear in other groups. In expanded row - adds an asterisk
column to highlight unique pairs among groups.

<img width="1384" alt="image"
src="https://user-images.githubusercontent.com/6446462/211634826-b7b88542-07c6-4c22-8bcc-3dec37cf90eb.png">

Co-authored-by: kibanamachine <[email protected]>
@peteharverson peteharverson changed the title [ML] AIOps: highlight pairs unique to groups in expanded row [ML] Explain Log Rate Spikes: highlight field pairs unique to groups in expanded row Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

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

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