Skip to content

Commit

Permalink
[ML] Remove dependency cache. (#189729)
Browse files Browse the repository at this point in the history
## Summary

Fixes #153477.
Fixes #153476.
Part of #187772 (technical debt).
Part of #153288 (migrate enzyme tests to react-testing-lib).

Removes dependency cache. The major culprit making this PR large and not
easy to split is that `getHttp()` from the dependency cache was used
throughout the code base for services like `mlJobService` and
`ml/mlApiServices` which then themselves were directly imported and not
part of React component lifecycles.

- For functional components this means mostly migrating to hooks that
allow accessing services.
- We still have a bit of a mix of usage of `withKibana` and `context`
for class based React components. This was not consolidated in this PR,
I took what's there and adjusted how services get used. These components
access services via `this.props.kibana.services.*` or
`this.context.services.*`.
- Functions no longer access the global services provided via dependency
cache but were updated to receive services via arguments.
- Stateful services like `mlJobService` are exposed now via a factory
that makes sure the service gets instantiated only once.
- Some tests where the mocks needed quite some refactoring were ported
to `react-testing-lib`. They no longer make use of snapshots or call
component methods which should be considered implementation details.
- We have a mix of usage of the plain `toasts` via `useMlKibana` and our
own `toastNotificationServices` that wraps `toasts`. I didn't
consolidate this in this PR but used what's available for the given
code.
- For class based components, service initializations were moved from
`componentDidMount()` to `constructor()` where I spotted it.
- We have a bit of a mix of naming: `ml`, `mlApiServices`,
`useMlApiContext()` for the same thing. I didn't consolidate the naming
in this PR, to avoid making this PR even larger. This can be done in a
follow up, once this PR is in this should be more straightforward and
less risky.
- Turns out `explorer_chart_config_builder.js` is no longer used
anywhere so I deleted it.
- `jobs/jobs_list/components/utils.d.ts` was missing some definitions,
tried to fix them.
- Moved `stashJobForCloning` to be a method of `mlJobService`.
- The `MetricSelector` component was an exact copy besides the i18n
label, consolidated that so anomaly detection wizards use the same
component.

### Checklist

- [x] [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
- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
walterra committed Aug 29, 2024
1 parent b79f5ab commit 38f4aa0
Show file tree
Hide file tree
Showing 247 changed files with 2,565 additions and 3,136 deletions.
16 changes: 1 addition & 15 deletions x-pack/plugins/ml/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import useObservable from 'react-use/lib/useObservable';
import type { ExperimentalFeatures, MlFeatures } from '../../common/constants/app';
import { ML_STORAGE_KEYS } from '../../common/types/storage';
import type { MlSetupDependencies, MlStartDependencies } from '../plugin';
import { clearCache, setDependencyCache } from './util/dependency_cache';
import { setLicenseCache } from './license';
import { MlRouter } from './routing';
import type { PageDependencies } from './routing/router';
Expand Down Expand Up @@ -97,7 +96,7 @@ const App: FC<AppProps> = ({
uiActions: deps.uiActions,
unifiedSearch: deps.unifiedSearch,
usageCollection: deps.usageCollection,
mlServices: getMlGlobalServices(coreStart.http, deps.data.dataViews, deps.usageCollection),
mlServices: getMlGlobalServices(coreStart, deps.data.dataViews, deps.usageCollection),
};
}, [deps, coreStart]);

Expand Down Expand Up @@ -160,18 +159,6 @@ export const renderApp = (
mlFeatures: MlFeatures,
experimentalFeatures: ExperimentalFeatures
) => {
setDependencyCache({
timefilter: deps.data.query.timefilter,
fieldFormats: deps.fieldFormats,
config: coreStart.uiSettings!,
docLinks: coreStart.docLinks!,
toastNotifications: coreStart.notifications.toasts,
recentlyAccessed: coreStart.chrome!.recentlyAccessed,
application: coreStart.application,
http: coreStart.http,
maps: deps.maps,
});

appMountParams.onAppLeave((actions) => actions.default());

ReactDOM.render(
Expand All @@ -187,7 +174,6 @@ export const renderApp = (
);

return () => {
clearCache();
ReactDOM.unmountComponentAtNode(appMountParams.element);
deps.data.search.session.clear();
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,11 @@ export function checkGetManagementMlJobsResolver({ mlCapabilities }: MlGlobalSer
}

export function checkCreateJobsCapabilitiesResolver(
mlApiServices: MlApiServices,
redirectToJobsManagementPage: () => Promise<void>
): Promise<MlCapabilities> {
return new Promise((resolve, reject) => {
getCapabilities()
getCapabilities(mlApiServices)
.then(async ({ capabilities, isPlatinumOrTrialLicense }) => {
_capabilities = capabilities;
// if the license is basic (isPlatinumOrTrialLicense === false) then do not redirect,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
* 2.0.
*/

import { ml } from '../services/ml_api_service';
import type { MlApiServices } from '../services/ml_api_service';

import type { MlCapabilitiesResponse } from '../../../common/types/capabilities';

export function getCapabilities(): Promise<MlCapabilitiesResponse> {
export function getCapabilities(ml: MlApiServices): Promise<MlCapabilitiesResponse> {
return ml.checkMlCapabilities();
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,27 @@ import useObservable from 'react-use/lib/useObservable';
import mockAnnotations from '../annotations_table/__mocks__/mock_annotations.json';
import React from 'react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import type { CoreStart } from '@kbn/core/public';
import { __IntlProvider as IntlProvider } from '@kbn/i18n-react';
import { createKibanaReactContext } from '@kbn/kibana-react-plugin/public';

import type { Annotation } from '../../../../../common/types/annotations';
import { AnnotationUpdatesService } from '../../../services/annotations_service';

import { AnnotationFlyout } from '.';
import { MlAnnotationUpdatesContext } from '../../../contexts/ml/ml_annotation_updates_context';

jest.mock('../../../util/dependency_cache', () => ({
getToastNotifications: () => ({ addSuccess: jest.fn(), addDanger: jest.fn() }),
}));
const kibanaReactContextMock = createKibanaReactContext({
mlServices: {
mlApiServices: {
annotations: {
indexAnnotation: jest.fn().mockResolvedValue({}),
deleteAnnotation: jest.fn().mockResolvedValue({}),
},
},
},
notifications: { toasts: { addDanger: jest.fn(), addSuccess: jest.fn() } },
} as unknown as Partial<CoreStart>);

const MlAnnotationUpdatesContextProvider = ({
annotationUpdatesService,
Expand All @@ -30,7 +40,9 @@ const MlAnnotationUpdatesContextProvider = ({
}) => {
return (
<MlAnnotationUpdatesContext.Provider value={annotationUpdatesService}>
<IntlProvider>{children}</IntlProvider>
<IntlProvider>
<kibanaReactContextMock.Provider>{children}</kibanaReactContextMock.Provider>
</IntlProvider>
</MlAnnotationUpdatesContext.Provider>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
import type { CommonProps } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
import { context } from '@kbn/kibana-react-plugin/public';
import { type MlPartitionFieldsType, ML_PARTITION_FIELDS } from '@kbn/ml-anomaly-utils';
import {
ANNOTATION_MAX_LENGTH_CHARS,
Expand All @@ -42,13 +43,12 @@ import type {
import { annotationsRefreshed } from '../../../services/annotations_service';
import { AnnotationDescriptionList } from '../annotation_description_list';
import { DeleteAnnotationModal } from '../delete_annotation_modal';
import { ml } from '../../../services/ml_api_service';
import { getToastNotifications } from '../../../util/dependency_cache';
import {
getAnnotationFieldName,
getAnnotationFieldValue,
} from '../../../../../common/types/annotations';
import { MlAnnotationUpdatesContext } from '../../../contexts/ml/ml_annotation_updates_context';
import type { MlKibanaReactContextValue } from '../../../contexts/kibana';

interface ViewableDetector {
index: number;
Expand Down Expand Up @@ -78,6 +78,9 @@ interface State {
}

export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
static contextType = context;
declare context: MlKibanaReactContextValue;

private deletionInProgress = false;

public state: State = {
Expand Down Expand Up @@ -126,14 +129,15 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
if (this.deletionInProgress) return;

const { annotationState } = this.state;
const toastNotifications = getToastNotifications();

if (annotationState === null || annotationState._id === undefined) {
return;
}

this.deletionInProgress = true;

const ml = this.context.services.mlServices.mlApiServices;
const toastNotifications = this.context.services.notifications.toasts;
try {
await ml.annotations.deleteAnnotation(annotationState._id);
toastNotifications.addSuccess(
Expand Down Expand Up @@ -237,11 +241,12 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
annotation.event = annotation.event ?? ANNOTATION_EVENT_USER;
annotationUpdatesService.setValue(null);

const ml = this.context.services.mlServices.mlApiServices;
const toastNotifications = this.context.services.notifications.toasts;
ml.annotations
.indexAnnotation(annotation)
.then(() => {
annotationsRefreshed();
const toastNotifications = getToastNotifications();
if (typeof annotation._id === 'undefined') {
toastNotifications.addSuccess(
i18n.translate(
Expand All @@ -265,7 +270,6 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
}
})
.catch((resp) => {
const toastNotifications = getToastNotifications();
if (typeof annotation._id === 'undefined') {
toastNotifications.addDanger(
i18n.translate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ import {
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
import { withKibana } from '@kbn/kibana-react-plugin/public';

import { addItemToRecentlyAccessed } from '../../../util/recently_accessed';
import { ml } from '../../../services/ml_api_service';
import { mlJobService } from '../../../services/job_service';
import { mlJobServiceFactory } from '../../../services/job_service';
import { toastNotificationServiceProvider } from '../../../services/toast_notification_service';
import { mlTableService } from '../../../services/table_service';
import { ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE } from '../../../../../common/constants/search';
import {
Expand All @@ -45,7 +46,6 @@ import {
ANNOTATION_EVENT_USER,
ANNOTATION_EVENT_DELAYED_DATA,
} from '../../../../../common/constants/annotations';
import { withKibana } from '@kbn/kibana-react-plugin/public';
import { ML_APP_LOCATOR, ML_PAGES } from '../../../../../common/constants/locator';
import { timeFormatter } from '@kbn/ml-date-utils';
import { MlAnnotationUpdatesContext } from '../../../contexts/ml/ml_annotation_updates_context';
Expand Down Expand Up @@ -90,10 +90,8 @@ class AnnotationsTableUI extends Component {
queryText: `event:(${ANNOTATION_EVENT_USER} or ${ANNOTATION_EVENT_DELAYED_DATA})`,
searchError: undefined,
jobId:
Array.isArray(this.props.jobs) &&
this.props.jobs.length > 0 &&
this.props.jobs[0] !== undefined
? this.props.jobs[0].job_id
Array.isArray(props.jobs) && props.jobs.length > 0 && props.jobs[0] !== undefined
? props.jobs[0].job_id
: undefined,
datafeedFlyoutVisible: false,
modelSnapshot: null,
Expand All @@ -103,6 +101,10 @@ class AnnotationsTableUI extends Component {
this.sorting = {
sort: { field: 'timestamp', direction: 'asc' },
};
this.mlJobService = mlJobServiceFactory(
toastNotificationServiceProvider(props.kibana.services.notifications.toasts),
props.kibana.services.mlServices.mlApiServices
);
}

getAnnotations() {
Expand All @@ -113,6 +115,8 @@ class AnnotationsTableUI extends Component {
isLoading: true,
});

const ml = this.props.kibana.services.mlServices.mlApiServices;

if (dataCounts.processed_record_count > 0) {
// Load annotations for the selected job.
ml.annotations
Expand Down Expand Up @@ -177,7 +181,7 @@ class AnnotationsTableUI extends Component {
}
}

return mlJobService.getJob(jobId);
return this.mlJobService.getJob(jobId);
}

annotationsRefreshSubscription = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { AnomalyDetails } from './anomaly_details';

import { mlTableService } from '../../services/table_service';
import { RuleEditorFlyout } from '../rule_editor';
import { ml } from '../../services/ml_api_service';
import { INFLUENCERS_LIMIT, ANOMALIES_TABLE_TABS, MAX_CHARS } from './anomalies_table_constants';

export class AnomaliesTableInternal extends Component {
Expand Down Expand Up @@ -69,6 +68,7 @@ export class AnomaliesTableInternal extends Component {
}

toggleRow = async (item, tab = ANOMALIES_TABLE_TABS.DETAILS) => {
const ml = this.context.services.mlServices.mlApiServices;
const itemIdToExpandedRowMap = { ...this.state.itemIdToExpandedRowMap };
if (itemIdToExpandedRowMap[item.rowId]) {
delete itemIdToExpandedRowMap[item.rowId];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,13 @@ import { parseInterval } from '../../../../common/util/parse_interval';
import { ML_APP_LOCATOR, ML_PAGES } from '../../../../common/constants/locator';
import { getFiltersForDSLQuery } from '../../../../common/util/job_utils';

import { mlJobService } from '../../services/job_service';
import { ml } from '../../services/ml_api_service';
import { useMlJobService } from '../../services/job_service';
import { escapeKueryForFieldValuePair, replaceStringTokens } from '../../util/string_utils';
import { getUrlForRecord, openCustomUrlWindow } from '../../util/custom_url_utils';
import type { SourceIndicesWithGeoFields } from '../../explorer/explorer_utils';
import { escapeDoubleQuotes, getDateFormatTz } from '../../explorer/explorer_utils';
import { usePermissionCheck } from '../../capabilities/check_capabilities';
import { useMlKibana } from '../../contexts/kibana';
import { useMlApiContext, useMlKibana } from '../../contexts/kibana';
import { useMlIndexUtils } from '../../util/index_service';

import { getQueryStringForInfluencers } from './get_query_string_for_influencers';
Expand Down Expand Up @@ -101,13 +100,24 @@ export const LinksMenuUI = (props: LinksMenuProps) => {

const kibana = useMlKibana();
const {
services: { data, share, application, uiActions },
services: {
data,
share,
application,
uiActions,
uiSettings,
notifications: { toasts },
},
} = kibana;
const { getDataViewById, getDataViewIdFromName } = useMlIndexUtils();
const ml = useMlApiContext();
const mlJobService = useMlJobService();

const job = useMemo(() => {
if (props.selectedJob !== undefined) return props.selectedJob;
return mlJobService.getJob(props.anomaly.jobId);
// skip mlJobService from deps
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.anomaly.jobId, props.selectedJob]);

const categorizationFieldName = job.analysis_config.categorization_field_name;
Expand Down Expand Up @@ -145,7 +155,9 @@ export const LinksMenuUI = (props: LinksMenuProps) => {

const getAnomaliesMapsLink = async (anomaly: MlAnomaliesTableRecord) => {
const initialLayers = getInitialAnomaliesLayers(anomaly.jobId);
const anomalyBucketStartMoment = moment(anomaly.source.timestamp).tz(getDateFormatTz());
const anomalyBucketStartMoment = moment(anomaly.source.timestamp).tz(
getDateFormatTz(uiSettings)
);
const anomalyBucketStart = anomalyBucketStartMoment.toISOString();
const anomalyBucketEnd = anomalyBucketStartMoment
.add(anomaly.source.bucket_span, 'seconds')
Expand Down Expand Up @@ -186,7 +198,9 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
sourceIndicesWithGeoFields[anomaly.jobId]
);
// Widen the timerange by one bucket span on start/end to increase chances of always having data on the map
const anomalyBucketStartMoment = moment(anomaly.source.timestamp).tz(getDateFormatTz());
const anomalyBucketStartMoment = moment(anomaly.source.timestamp).tz(
getDateFormatTz(uiSettings)
);
const anomalyBucketStart = anomalyBucketStartMoment
.subtract(anomaly.source.bucket_span, 'seconds')
.toISOString();
Expand Down Expand Up @@ -513,7 +527,6 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
.catch((resp) => {
// eslint-disable-next-line no-console
console.log('openCustomUrl(): error loading categoryDefinition:', resp);
const { toasts } = kibana.services.notifications;
toasts.addDanger(
i18n.translate('xpack.ml.anomaliesTable.linksMenu.unableToOpenLinkErrorMessage', {
defaultMessage:
Expand Down Expand Up @@ -615,7 +628,6 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
if (job === undefined) {
// eslint-disable-next-line no-console
console.log(`viewExamples(): no job found with ID: ${props.anomaly.jobId}`);
const { toasts } = kibana.services.notifications;
toasts.addDanger(
i18n.translate('xpack.ml.anomaliesTable.linksMenu.unableToViewExamplesErrorMessage', {
defaultMessage: 'Unable to view examples as no details could be found for job ID {jobId}',
Expand Down Expand Up @@ -702,7 +714,6 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
.catch((resp) => {
// eslint-disable-next-line no-console
console.log('viewExamples(): error loading categoryDefinition:', resp);
const { toasts } = kibana.services.notifications;
toasts.addDanger(
i18n.translate('xpack.ml.anomaliesTable.linksMenu.loadingDetailsErrorMessage', {
defaultMessage:
Expand Down Expand Up @@ -736,7 +747,6 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
`viewExamples(): error finding type of field ${categorizationFieldName} in indices:`,
datafeedIndices
);
const { toasts } = kibana.services.notifications;
toasts.addDanger(
i18n.translate('xpack.ml.anomaliesTable.linksMenu.noMappingCouldBeFoundErrorMessage', {
defaultMessage:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ import {
ANOMALY_DETECTION_DEFAULT_TIME_RANGE,
ANOMALY_DETECTION_ENABLE_TIME_RANGE,
} from '../../../../common/constants/settings';
import { mlJobService } from '../../services/job_service';
import { useMlJobService } from '../../services/job_service';

export const useCreateADLinks = () => {
const {
services: {
http: { basePath },
},
} = useMlKibana();
const mlJobService = useMlJobService();

const useUserTimeSettings = useUiSettings().get(ANOMALY_DETECTION_ENABLE_TIME_RANGE);
const userTimeSettings = useUiSettings().get(ANOMALY_DETECTION_DEFAULT_TIME_RANGE);
Expand Down
Loading

0 comments on commit 38f4aa0

Please sign in to comment.