-
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
[ML] Anomaly Detection: Adds single metric viewer embeddable for dashboards #175857
[ML] Anomaly Detection: Adds single metric viewer embeddable for dashboards #175857
Conversation
x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_initializer.tsx
Show resolved
Hide resolved
x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_initializer.tsx
Show resolved
Hide resolved
x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_initializer.tsx
Show resolved
Hide resolved
...ins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable.tsx
Show resolved
Hide resolved
...imeseriesexplorer/timeseriesexplorer_embeddable_chart/timeseriesexplorer_embeddable_chart.js
Outdated
Show resolved
Hide resolved
scheduledEvents: any; | ||
showForecastCheckbox?: any; | ||
showForecastCheckbox?: boolean; |
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 you have forecast data showing up? I can't find a way of displaying data from a forecast in a chart. It might be best left for future work - it isn't needed for this first version, and you'd need to add a control into the initial modal for displaying which forecast to display.
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.
Forecasting will be added in a follow up - though I left some of the code that will be needed for it since we will need it anyway.
x-pack/plugins/ml/public/embeddables/single_metric_viewer/single_metric_viewer_embeddable.tsx
Show resolved
Hide resolved
mlApiServices.results | ||
.anomalySearch( | ||
{ | ||
// @ts-ignore |
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.
We can probably remove these ts-ignore for the size
param, or update it to ts-expect-error if we are expecting the type to be fixed in the future.
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.
Added ts-expect error in c5181f1
@@ -7,17 +7,18 @@ | |||
|
|||
import { mlJobService } from '../services/job_service'; | |||
import { Entity } from './components/entity_control/entity_control'; | |||
import { JobId } from '../../../common/types/anomaly_detection_jobs'; | |||
import { JobId, CombinedJob } from '../../../common/types/anomaly_detection_jobs'; |
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.
nit: import type {}
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.
Updated in c5181f1
} | ||
|
||
private async getServices(): Promise<SingleMetricViewerEmbeddableServices> { | ||
const [coreStart, pluginsStart] = await this.getStartServices(); |
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.
All of these awaited imports can be put inside a Promise.all
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 catch - updated in c5181f1
@@ -279,7 +283,12 @@ class TimeseriesChartIntl extends Component { | |||
chartElement.selectAll('*').remove(); | |||
|
|||
if (typeof selectedJob !== 'undefined') { | |||
this.fieldFormat = mlFieldFormatService.getFieldFormat(selectedJob.job_id, detectorIndex); | |||
this.fieldFormat = this.context?.services?.mlServices?.mlFieldFormatService |
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.
As we have the context
here it looks like we can just instantiate mlFieldFormatService
inside this class for both the embeddable and original use.
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.
This one might be a bit trickier - I'll add it to the follow up tasks in the meta issue 👍
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.
ok cool.
I didn't realise that mlFieldFormatService
has state and stores the formats per job across components.
I think that should be changed in the future.
@@ -70,7 +72,12 @@ export interface MlServicesContext { | |||
mlServices: MlGlobalServices; | |||
} | |||
|
|||
export type MlGlobalServices = ReturnType<typeof getMlGlobalServices>; | |||
export type MlGlobalServices = ReturnType<typeof getMlGlobalServices> & { | |||
mlUtilsService?: { |
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.
It looks like we can probably get rid of this mlUtilsService
and instantiate the providers for mlTimeBuckets
and mlTimeSeriesExplorer
inside the react classes.
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.
Done in c5181f1
@@ -47,7 +49,7 @@ export const TimeSeriesChartWithTooltips: FC<TimeSeriesChartWithTooltipsProps> = | |||
const { toasts: toastNotifications } = useNotifications(); | |||
const { | |||
services: { | |||
mlServices: { mlApiServices }, | |||
mlServices: { mlApiServices, mlUtilsService }, |
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.
rather than using this mlUtilsService
here, we can call timeBucketsProvider
and pass it uiSettings
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.
As per our chat, removed the need to have mlUtilsService
in context at all - I was able to call the relevant providers and pass them the needed services already kept in context.
Changes in c5181f1
// @ts-ignore | ||
size: 1, | ||
body: { | ||
query: { |
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.
Looking into this more, we can probably get ride of the typescript error entirely if we put size
param in the body {}:
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 taking a look - I gave that a try but that seems to make the size
property ignored by the request. Up for digging into it but seems outside the scope of this PR as these requests have just been copied over and have been working as is.
|
||
export function useSingleMetricViwerInputResolver( | ||
embeddableInput: Observable<SingleMetricViewerEmbeddableInput>, | ||
refresh: Observable<any>, |
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.
Looks like this could be Observable<void>
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.
Updated in f682daa
embeddableContext: InstanceType<ISingleMetricViewerEmbeddable>; | ||
embeddableInput: Observable<SingleMetricViewerEmbeddableInput>; | ||
services: SingleMetricViewerEmbeddableServices; | ||
refresh: Observable<any>; |
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.
Looks like this could be Observable<void>
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.
Updated in f682daa
@@ -279,7 +283,12 @@ class TimeseriesChartIntl extends Component { | |||
chartElement.selectAll('*').remove(); | |||
|
|||
if (typeof selectedJob !== 'undefined') { | |||
this.fieldFormat = mlFieldFormatService.getFieldFormat(selectedJob.job_id, detectorIndex); | |||
this.fieldFormat = this.context?.services?.mlServices?.mlFieldFormatService |
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.
ok cool.
I didn't realise that mlFieldFormatService
has state and stores the formats per job across components.
I think that should be changed in the future.
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.
Gave it another cycle of testing and overall looking good. Only issue for me is sorting out the issue with metric
detectors.
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.
Looks good overall, just added some comments about naming! Good job digging through all the legacy code and get this to work!
It would be great if you could add comments in the code that indicate where code has been duplicated from and a corresponding issue that has references these comments. We did something similar for AIOps and Data Drift (see #174377).
...imeseriesexplorer/timeseriesexplorer_embeddable_chart/timeseriesexplorer_embeddable_chart.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/util/timeseriesexplorer_utils.ts
Outdated
Show resolved
Hide resolved
Updated in f682daa |
This has been updated with all comments and is ready for a final look when you get a chance cc @peteharverson, @jgowdyelastic, @walterra I did plenty of testing but as this is a large change I would appreciate some help on testing from a couple of other people if possible. 🙏 |
Once you merge |
x-pack/plugins/ml/public/application/util/timeseriesexplorer_utils.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/services/field_format_service_provider.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/util/index_utils_provider.ts
Outdated
Show resolved
Hide resolved
...pplication/timeseriesexplorer/timeseriesexplorer_utils/timeseries_search_service_provider.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
x-pack/plugins/ml/public/application/services/forecast_service_provider.ts
Show resolved
Hide resolved
// Note: This is a duplicated of `timeseries_search_service.ts` updated to move away from dependency cache. | ||
// The original file will be removed once all references are replaced with references to this file. |
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.
Suggest to replace comment with the following so we can search for the pattern // TODO Consolidate
:
// TODO Consolidate with legacy code in
// `x-pack/plugins/ml/public/application/timeseriesexplorer/timeseries_search_service.ts`
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
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.
Let's add a comment here like:
// TODO Consolidate with legacy code in
// `ml/public/application/timeseriesexplorer/timeseriesexplorer_utils/timeseriesexplorer_utils.js`.
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.
All comments for consolidation added in 1bd6b8b
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 agree with @walterra 's suggestion about tracking duplicate code using comments in the source, as they'll be easier to find later on.
Other than that, LGTM
@elasticmachine merge upstream |
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.
Latest changes LGTM! 🥳
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…oards (elastic#175857) ## Summary Related issue to [add ability to insert "Single Metric Viewer" into a dashboard](elastic#173555) This PR adds the single metric viewer as an embeddable that can be added to dashboards. ### NOTE FOR TESTING: This PR relies on the SMV fix for 'metric' jobs elastic#176354 If that fix has not been merged, you will need to find `getAnomalyRecordsSchema` definition and add `functionDescription: schema.maybe(schema.nullable(schema.string())),` to it for local testing. ### Screenshots of feature <img width="698" alt="image" src="https://github.com/elastic/kibana/assets/6446462/425e701a-3c9d-4a82-bf2e-1db5b3689165"> <img width="1193" alt="image" src="https://github.com/elastic/kibana/assets/6446462/e941ec1c-14f6-4723-b80c-71124f617dc9"> <img width="1209" alt="image" src="https://github.com/elastic/kibana/assets/6446462/dddd1dde-844c-47ae-ba94-61de5301746f"> <img width="1214" alt="image" src="https://github.com/elastic/kibana/assets/6446462/39439b4f-d296-4f3d-bdc9-4922553af6fa"> ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…oards (elastic#175857) ## Summary Related issue to [add ability to insert "Single Metric Viewer" into a dashboard](elastic#173555) This PR adds the single metric viewer as an embeddable that can be added to dashboards. ### NOTE FOR TESTING: This PR relies on the SMV fix for 'metric' jobs elastic#176354 If that fix has not been merged, you will need to find `getAnomalyRecordsSchema` definition and add `functionDescription: schema.maybe(schema.nullable(schema.string())),` to it for local testing. ### Screenshots of feature <img width="698" alt="image" src="https://github.com/elastic/kibana/assets/6446462/425e701a-3c9d-4a82-bf2e-1db5b3689165"> <img width="1193" alt="image" src="https://github.com/elastic/kibana/assets/6446462/e941ec1c-14f6-4723-b80c-71124f617dc9"> <img width="1209" alt="image" src="https://github.com/elastic/kibana/assets/6446462/dddd1dde-844c-47ae-ba94-61de5301746f"> <img width="1214" alt="image" src="https://github.com/elastic/kibana/assets/6446462/39439b4f-d296-4f3d-bdc9-4922553af6fa"> ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…oards (elastic#175857) ## Summary Related issue to [add ability to insert "Single Metric Viewer" into a dashboard](elastic#173555) This PR adds the single metric viewer as an embeddable that can be added to dashboards. ### NOTE FOR TESTING: This PR relies on the SMV fix for 'metric' jobs elastic#176354 If that fix has not been merged, you will need to find `getAnomalyRecordsSchema` definition and add `functionDescription: schema.maybe(schema.nullable(schema.string())),` to it for local testing. ### Screenshots of feature <img width="698" alt="image" src="https://github.com/elastic/kibana/assets/6446462/425e701a-3c9d-4a82-bf2e-1db5b3689165"> <img width="1193" alt="image" src="https://github.com/elastic/kibana/assets/6446462/e941ec1c-14f6-4723-b80c-71124f617dc9"> <img width="1209" alt="image" src="https://github.com/elastic/kibana/assets/6446462/dddd1dde-844c-47ae-ba94-61de5301746f"> <img width="1214" alt="image" src="https://github.com/elastic/kibana/assets/6446462/39439b4f-d296-4f3d-bdc9-4922553af6fa"> ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Related issue to add ability to insert "Single Metric Viewer" into a dashboard
This PR adds the single metric viewer as an embeddable that can be added to dashboards.
NOTE FOR TESTING:
This PR relies on the SMV fix for 'metric' jobs #176354
If that fix has not been merged, you will need to find
getAnomalyRecordsSchema
definition and addfunctionDescription: schema.maybe(schema.nullable(schema.string())),
to it for local testing.Screenshots of feature
Checklist
Delete any items that are not applicable to this PR.