-
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
[Logs UI] Add dataset-specific categorization warnings #75351
[Logs UI] Add dataset-specific categorization warnings #75351
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
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.
Operations LGTM, just fixes the location of the infra storybook
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.
Thanks for the adding the component to storybook! It was useful for the first pass 👍
I have a comment regarding the layout. When the description of an issue in a dataset is too long, it wraps to the next line. If a dataset has several issues it looks a bit chaotic.
Would it make sense to group the warnings per dataset? Something like:
**first.dataset**
• The analysis couldn't extract more than a single category from the log messages.
• 95% of the categories only rarely have messages assigned to them.
• The ratio of categories per analyzed document is very high with 0.7.
**second.dataset**
• None of the extracted categories frequently have messages assigned to them.
• 70% of the categories won't have new messages assigned to them because they are overshadowed by less specific categories.
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.
I did a first pass on the code. I left some small comments.
reasons: CategoryQualityWarningReason[]; | ||
} | ||
|
||
export type QualityWarning = CategoryQualityWarning; |
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.
Was there a reason to keep this alias? Otherwise I think we can safely remove it.
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.
Well, the thinking was that the CategoryQualityWarning
is just a special case. I wanted to make that clear in the consuming code that it shouldn't rely too strictly on the fact that no other warnings have been implemented.
Other warnings could be that buckets have been skipped due to delay, which can happen in any anomaly detection job.
.../infra/public/components/logging/log_analysis_job_status/quality_warning_notices.stories.tsx
Show resolved
Hide resolved
.../plugins/infra/public/components/logging/log_analysis_job_status/quality_warning_notices.tsx
Show resolved
Hide resolved
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.
Code looks alright! I found a crash on an API call (see comment below).
Since we are creating the types ourselves I wonder if there are other things we missed. Maybe the ML team has shared types that we can reuse.
x-pack/plugins/infra/public/containers/logs/log_analysis/api/ml_get_jobs_summary_api.ts
Outdated
Show resolved
Hide resolved
I added patches for some of the comments I had. |
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.
I addressed the changes I requested... so I approve my own changes!
@afgomez thanks for the review and the grouping suggestions. Unfortunately the |
@afgomez I modified the DOM structure to use a definition list, which looks almost the same as your solution. But it doesn't have the margin problem and doesn't skip heading levels for accessibility. I updated the preview in the description. Does this look good to you? |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
This adds dataset-specific categorization warnings for the categorization module. The warnings are displayed in call-outs on the relevant tabs as well as the job setup screens if a prior job with warnings exists. To that end this also changes the categorization job configuration to enable the partitioned categorization mode. Co-authored-by: Alejandro Fernández Gómez <[email protected]>
Summary
This adds dataset-specific categorization warnings for the categorization module. The warnings are displayed in call-outs on the relevant tabs as well as the job setup screens if a prior job with warnings exists. To that end this also changes the categorization job configuration to enable the partitioned categorization mode.
closes #60392
Previews
Implementation notes
This also replaces the locally defined setup fly-out on the categories tab with the shared fly-out. To avoid leaking the log rate analysis module into that tab the fly-out can now be configured with an allow-list of modules to display.
@elastic/kibana-operations
and@elastic/ml-ui
were tagged as codeowners on this PR because I fixed one line in the storybook configuration and updated one of the ml module definitions created for the Logs UI.Testing
yarn storybook infra
to build it.