-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add Anomaly Explorer charts embeddable #94396
Conversation
…lorer_service to anomaly_explorer_charts_service
I've noticed that the horizontal scroll bar is always presented, we need to fix some CSS |
I think the Severity threshold value should persist. At the moment it always falls back to the "minor" after a page refresh. |
x-pack/plugins/ml/public/ui_actions/open_in_anomaly_explorer_action.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ui_actions/open_in_anomaly_explorer_action.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ui_actions/apply_entity_filters_action.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ui_actions/apply_entity_filters_action.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ui_actions/apply_entity_filters_action.tsx
Outdated
Show resolved
Hide resolved
|
||
return fullSeriesConfig; | ||
} | ||
public async getCombinedJobs(jobIds: string[]): Promise<CombinedJobWithStats[]> { |
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.
do we need this method? it doesn't do anything with the response
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 updated this getCombinedJobs
a bit here now 47509f8, mainly to avoid getting unnecessary stats. However, considering that this is a charts service, maybe this would be better in AnomalyDetectorService
?
x-pack/plugins/ml/public/application/services/anomaly_explorer_charts_service.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/explorer/explorer_utils.js
Outdated
Show resolved
Hide resolved
...rer/explorer_charts/components/explorer_chart_label/influencer_filters/influencer_filter.tsx
Outdated
Show resolved
Hide resolved
...rer/explorer_charts/components/explorer_chart_label/influencer_filters/influencer_filter.tsx
Outdated
Show resolved
Hide resolved
I'd recommend writing some unit tests for the input resolver. It's the most important and complex part of this feature |
I have added a fix for that issue here |
...ion/explorer/explorer_charts/components/explorer_chart_label/entity_filter/entity_filter.tsx
Show resolved
Hide resolved
The sizing behavior is much better now. I still see the occasional odd behavior, such as when maximizing the window, when the x axis time range doesn't obey the time range in the dashboard. But I'm happy for any other changes in this area to be done in a follow up. |
I have added a note for that on the meta issue here #94650 |
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.
Good to see functional tests coming in as part of this PR! 🎉
Left a few comments and suggestions.
x-pack/test/functional/apps/ml/embeddables/anomaly_charts_dashboard_embeddables.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/embeddables/anomaly_charts_dashboard_embeddables.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/embeddables/anomaly_charts_dashboard_embeddables.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/embeddables/anomaly_charts_dashboard_embeddables.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/services/ml/dashboard_job_selection_table.ts
Outdated
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.
Tested latest changes and LGTM.
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.
Assuming unresolved comments will be addressed in a follow up PR, LGTM! Great job 🚀
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.
Functional tests LGTM 🎉
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/kpi_network·ts.apis SecuritySolution Endpoints Kpi Network With filebeat Make sure that we get KpiNetwork networkEvents dataStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @qn895 |
💔 Backport failed❌ 7.x: Commit could not be cherrypicked due to conflicts To backport manually run: |
# Conflicts: # x-pack/test/accessibility/config.ts
Summary
Adds the anomaly charts from the Anomaly Explorer as an embeddable to Kibana's Dashboard.
Initialize:
With query:
With filters:
With custom time range:
Context menu:
Need to do:
AnomalyExplorerChartsService
(formerlyexplorer_charts_container_service.test
)Checklist