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.

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: Fix anomaly marker click in Single Metric Viewer embeddable #176788

Merged
merged 25 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c07d8d7
fix anomaly marker click in SMV embeddable
walterra Feb 13, 2024
85dc3f2
remove console.logs
walterra Feb 13, 2024
c84fedc
dependencies
walterra Feb 13, 2024
9ff3807
simplify anomaly explorer context
walterra Feb 13, 2024
4abe2a9
add comment
walterra Feb 13, 2024
0ce7c8a
fix dependencies
walterra Feb 13, 2024
5e479f4
fix jest tests
walterra Feb 13, 2024
1512c2f
fix embeddable anomaly chart
walterra Feb 13, 2024
2fcc8b6
fix jest test
walterra Feb 13, 2024
02b1dfc
remove unused dependency cache items
walterra Feb 13, 2024
ac060ae
Merge branch 'main' into 176651-ml-smv-embeddable-click
walterra Feb 14, 2024
5508ae2
adds deprecation notices
walterra Feb 14, 2024
70ab770
cleanup
walterra Feb 14, 2024
ea30406
move mlIndexUtils out of global context
walterra Feb 14, 2024
ad49b54
add comments about services
walterra Feb 14, 2024
42e89c1
move loadDataViewListItems to mlIndexUtils
walterra Feb 14, 2024
f57c08d
destructure mlIndexUtils
walterra Feb 14, 2024
bccd244
don't return mlIndexUtils
walterra Feb 14, 2024
1a4f9b6
Merge branch 'main' into 176651-ml-smv-embeddable-click
walterra Feb 15, 2024
3ee830d
fix mlJobService
walterra Feb 15, 2024
9333edc
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Feb 15, 2024
3f40ece
Merge branch 'main' into 176651-ml-smv-embeddable-click
walterra Feb 15, 2024
bcda568
Merge branch '176651-ml-smv-embeddable-click' of github.com:walterra/…
walterra Feb 15, 2024
d4e6b66
Merge branch 'main' into 176651-ml-smv-embeddable-click
walterra Feb 19, 2024
cef71e5
Merge branch 'main' into 176651-ml-smv-embeddable-click
walterra Mar 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions x-pack/plugins/ml/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import './_index.scss';
import ReactDOM from 'react-dom';
import { pick } from 'lodash';

import type { DataViewsContract } from '@kbn/data-views-plugin/public';
import type { AppMountParameters, CoreStart, HttpStart } from '@kbn/core/public';
import type { UsageCollectionSetup } from '@kbn/usage-collection-plugin/public';
import { DatePickerContextProvider, type DatePickerDependencies } from '@kbn/ml-date-picker';
Expand All @@ -33,6 +34,8 @@ import { HttpService } from './services/http_service';
import type { PageDependencies } from './routing/router';
import { EnabledFeaturesContextProvider } from './contexts/ml';
import type { StartServices } from './contexts/kibana';
import { fieldFormatServiceFactory } from './services/field_format_service_factory';
import { indexServiceFactory } from './util/index_service';

export type MlDependencies = Omit<
MlSetupDependencies,
Expand All @@ -53,13 +56,20 @@ const localStorage = new Storage(window.localStorage);
/**
* Provides global services available across the entire ML app.
*/
export function getMlGlobalServices(httpStart: HttpStart, usageCollection?: UsageCollectionSetup) {
export function getMlGlobalServices(
httpStart: HttpStart,
dataViews: DataViewsContract,
usageCollection?: UsageCollectionSetup
) {
const httpService = new HttpService(httpStart);
const mlApiServices = mlApiServicesProvider(httpService);
const mlIndexUtils = indexServiceFactory(dataViews);
const mlFieldFormatService = fieldFormatServiceFactory(mlApiServices, mlIndexUtils);
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent bloat of the context, perhaps the mlFieldFormatService should be created in the component as needed and then passed/made available to it's children?

For example, we already have mlApiServices and dataViews in context, I believe.
fieldFormatServiceFactory requires mlApiServices and mlIndexUtils - which, in turn, only requires the dataViewsService.

From that, fieldFormatService could be instantiated once in the component in which it is needed instead of adding more to the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's bloat? Prior to this PR the service was only instantiated once too, it's just that with the dependency cache under the hood it could be imported everywhere without being passed around via context. So this variant felt closer to the original then instantiating multiple times. Note the factory doesn't have any costly side effects, for example it doesn't call any requests, that's only happening once you call one of its methods.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree that it would be better to keep mlIndexUtils and mlFieldFormatService out of this context as they are used so rarely throughout our plugin. It sets a precedence that all utils and services we have should go in here too.
Instantiating them when needed in each file would be better IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, I'm leaving mlFieldFormatService here as a global service for now because of its internal state. mlIndexUtils will not be passed on to global services, instead it can be accessed within components via useMlIndexUtils().

I added the following comment to each place where the services are set up (the main ML app, the anomaly charts embeddable and the single metric viewer embeddable):

    // Note on the following services:
    // - `mlIndexUtils` is just instantiated here to be passed on to `mlFieldFormatService`,
    //   but it's not being made available as part of global services. Since it's just
    //   some stateless utils `useMlIndexUtils()` should be used from within components.
    // - `mlFieldFormatService` is a stateful legacy service that relied on "dependency cache",
    //   so because of its own state it needs to be made available as a global service.
    //   In the long run we should again try to get rid of it here and make it available via
    //   its own context or possibly without having a singleton like state at all, since the
    //   way this manages its own state right now doesn't consider React component lifecycles.

Copy link
Member

Choose a reason for hiding this comment

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

Does this still require a code change? mlIndexUtils is still being added to the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, overlooked that one, thanks! Fixed in bccd244.


return {
httpService,
mlApiServices,
mlFieldFormatService,
mlUsageCollection: mlUsageCollectionProvider(usageCollection),
mlCapabilities: new MlCapabilitiesService(mlApiServices),
mlLicense: new MlLicense(),
Expand Down Expand Up @@ -106,7 +116,7 @@ const App: FC<AppProps> = ({ coreStart, deps, appMountParams, isServerless, mlFe
unifiedSearch: deps.unifiedSearch,
usageCollection: deps.usageCollection,
...coreStart,
mlServices: getMlGlobalServices(coreStart.http, deps.usageCollection),
mlServices: getMlGlobalServices(coreStart.http, deps.data.dataViews, deps.usageCollection),
};
}, [deps, coreStart]);

Expand Down Expand Up @@ -168,25 +178,13 @@ export const renderApp = (
setDependencyCache({
timefilter: deps.data.query.timefilter,
fieldFormats: deps.fieldFormats,
autocomplete: deps.unifiedSearch.autocomplete,
config: coreStart.uiSettings!,
chrome: coreStart.chrome!,
docLinks: coreStart.docLinks!,
toastNotifications: coreStart.notifications.toasts,
overlays: coreStart.overlays,
theme: coreStart.theme,
recentlyAccessed: coreStart.chrome!.recentlyAccessed,
basePath: coreStart.http.basePath,
savedSearch: deps.savedSearch,
application: coreStart.application,
http: coreStart.http,
security: deps.security,
dashboard: deps.dashboard,
maps: deps.maps,
dataVisualizer: deps.dataVisualizer,
dataViews: deps.data.dataViews,
share: deps.share,
lens: deps.lens,
});

appMountParams.onAppLeave((actions) => actions.default());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { EuiFlexGroup, EuiFlexItem, EuiInMemoryTable, EuiText } from '@elastic/e

import { FormattedMessage } from '@kbn/i18n-react';
import { usePageUrlState } from '@kbn/ml-url-state';
import { context } from '@kbn/kibana-react-plugin/public';

import { getColumns } from './anomalies_table_columns';

Expand All @@ -29,6 +30,8 @@ import { ml } from '../../services/ml_api_service';
import { INFLUENCERS_LIMIT, ANOMALIES_TABLE_TABS, MAX_CHARS } from './anomalies_table_constants';

export class AnomaliesTableInternal extends Component {
static contextType = context;

constructor(props) {
super(props);

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

const columns = getColumns(
this.context.services.mlServices.mlFieldFormatService,
tableData.anomalies,
tableData.jobIds,
tableData.examplesByJobId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ jest.mock('../../license', () => ({
jest.mock('../../capabilities/get_capabilities', () => ({
getCapabilities: () => {},
}));
jest.mock('../../services/field_format_service', () => ({
getFieldFormat: () => {},
}));
jest.mock('./links_menu', () => () => <div id="mocLinkCom">mocked link component</div>);
jest.mock('./description_cell', () => () => (
<div id="mockDescriptorCom">mocked description component</div>
Expand All @@ -31,6 +28,10 @@ jest.mock('./influencers_cell', () => () => (
<div id="mocInfluencerCom">mocked influencer component</div>
));

const mlFieldFormatServiceMock = {
getFieldFormat: () => {},
};

const columnData = {
items: mockAnomaliesTableData.default.anomalies,
jobIds: mockAnomaliesTableData.default.jobIds,
Expand All @@ -48,6 +49,7 @@ const columnData = {
describe('AnomaliesTable', () => {
test('all columns created', () => {
const columns = getColumns(
mlFieldFormatServiceMock,
columnData.items,
columnData.jobIds,
columnData.examplesByJobId,
Expand Down Expand Up @@ -103,6 +105,7 @@ describe('AnomaliesTable', () => {
};

const columns = getColumns(
mlFieldFormatServiceMock,
noEntityValueColumnData.items,
noEntityValueColumnData.jobIds,
noEntityValueColumnData.examplesByJobId,
Expand Down Expand Up @@ -133,6 +136,7 @@ describe('AnomaliesTable', () => {
};

const columns = getColumns(
mlFieldFormatServiceMock,
noInfluencersColumnData.items,
noInfluencersColumnData.jobIds,
noInfluencersColumnData.examplesByJobId,
Expand Down Expand Up @@ -163,6 +167,7 @@ describe('AnomaliesTable', () => {
};

const columns = getColumns(
mlFieldFormatServiceMock,
noActualColumnData.items,
noActualColumnData.jobIds,
noActualColumnData.examplesByJobId,
Expand Down Expand Up @@ -193,6 +198,7 @@ describe('AnomaliesTable', () => {
};

const columns = getColumns(
mlFieldFormatServiceMock,
noTypicalColumnData.items,
noTypicalColumnData.jobIds,
noTypicalColumnData.examplesByJobId,
Expand Down Expand Up @@ -223,6 +229,7 @@ describe('AnomaliesTable', () => {
};

const columns = getColumns(
mlFieldFormatServiceMock,
multipleJobIdsData.items,
multipleJobIdsData.jobIds,
multipleJobIdsData.examplesByJobId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { EntityCell } from '../entity_cell';
import { InfluencersCell } from './influencers_cell';
import { LinksMenu } from './links_menu';
import { checkPermission } from '../../capabilities/check_capabilities';
import { mlFieldFormatService } from '../../services/field_format_service';
import { formatValue } from '../../formatters/format_value';
import { INFLUENCERS_LIMIT, ANOMALIES_TABLE_TABS } from './anomalies_table_constants';
import { SeverityCell } from './severity_cell';
Expand Down Expand Up @@ -56,6 +55,7 @@ function showLinksMenuForItem(item, showViewSeriesLink, sourceIndicesWithGeoFiel
}

export function getColumns(
mlFieldFormatService,
items,
jobIds,
examplesByJobId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export const LinksMenuUI = (props: LinksMenuProps) => {

const getAnomaliesMapsLink = async (anomaly: MlAnomaliesTableRecord) => {
const index = job.datafeed_config.indices[0];
const dataViewId = await getDataViewIdFromName(index);
const dataViewId = await getDataViewIdFromName(data.dataViews, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned after James's comment below - the indexServiceFactory in ml/public/application/util/index_service.ts (which is meant to replace the utils file getDataViewIdFromName is pulled from here) should be used here to get that functionality. It only requires dataViewsService, which should be available in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 70ab770.


const initialLayers = getInitialAnomaliesLayers(anomaly.jobId);
const anomalyBucketStartMoment = moment(anomaly.source.timestamp).tz(getDateFormatTz());
Expand Down Expand Up @@ -144,7 +144,7 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
sourceIndicesWithGeoFields: SourceIndicesWithGeoFields
) => {
const index = job.datafeed_config.indices[0];
const dataViewId = await getDataViewIdFromName(index);
const dataViewId = await getDataViewIdFromName(data.dataViews, index);

// Create a layer for each of the geoFields
const initialLayers = getInitialSourceIndexFieldLayers(
Expand Down Expand Up @@ -218,7 +218,7 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
const getDataViewId = async () => {
const index = job.datafeed_config.indices[0];

const dataViewId = await getDataViewIdFromName(index, job);
const dataViewId = await getDataViewIdFromName(data.dataViews, index, job);

// If data view doesn't exist for some reasons
if (!dataViewId && !unmounted) {
Expand Down Expand Up @@ -657,7 +657,7 @@ export const LinksMenuUI = (props: LinksMenuProps) => {
// index configured in the datafeed. If a Kibana data view has not been created
// for this index, then the user will see a warning message on the Discover tab advising
// them that no matching data view has been configured.
const dataViewId = await getDataViewIdFromName(index);
const dataViewId = await getDataViewIdFromName(data.dataViews, index);

// We should not redirect to Discover if data view doesn't exist
if (!dataViewId) return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class CustomUrlsUI extends Component<CustomUrlsProps, CustomUrlsState> {
);
});

loadDataViewListItems()
loadDataViewListItems(this.props.kibana.services.data.dataViews)
.then((dataViewListItems) => {
this.setState({ dataViewListItems });
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export const kibanaContextMock = {
mlCapabilities: {
refreshCapabilities: jest.fn(),
},
mlFieldFormatService: {
getFieldFormat: jest.fn(),
},
},
notifications: notificationServiceMock.createStartContract(),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export const useResultsViewConfig = (jobId: string) => {

try {
const destIndex = getDestinationIndex(jobConfigUpdate);
const destDataViewId = (await getDataViewIdFromName(destIndex)) ?? destIndex;
const destDataViewId = (await getDataViewIdFromName(dataViews, destIndex)) ?? destIndex;
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 Feb 13, 2024

Choose a reason for hiding this comment

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

Same note here for either instantiating indexServiceFactory with the necessary services available via useMlKibana or passing in the service or function after instantiation wherever this hook is used.

This is the long term solution @jgowdyelastic and I chatted about for moving away from the dependency cache and also preventing making the context too bloated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 70ab770.

let fetchedDataView: DataView | undefined;

try {
Expand All @@ -115,7 +115,8 @@ export const useResultsViewConfig = (jobId: string) => {
if (fetchedDataView === undefined) {
setNeedsDestDataView(true);
const sourceIndex = jobConfigUpdate.source.index[0];
const sourceDataViewId = (await getDataViewIdFromName(sourceIndex)) ?? sourceIndex;
const sourceDataViewId =
(await getDataViewIdFromName(dataViews, sourceIndex)) ?? sourceIndex;
try {
fetchedDataView = await dataViews.get(sourceDataViewId);
} catch (e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export const Controls: FC<Props> = React.memo(

const {
services: {
data,
share,
application: { navigateToUrl, capabilities },
},
Expand Down Expand Up @@ -138,7 +139,7 @@ export const Controls: FC<Props> = React.memo(
const nodeType = selectedNode?.data('type');

const onCreateJobClick = useCallback(async () => {
const dataViewId = await getDataViewIdFromName(nodeLabel);
const dataViewId = await getDataViewIdFromName(data.dataViews, nodeLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 70ab770.


if (dataViewId !== null) {
const path = await mlLocator.getUrl({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
import { from } from 'rxjs';
import { map } from 'rxjs/operators';

import { mlFieldFormatService } from '../../services/field_format_service';
import type { MlFieldFormatService } from '../../services/field_format_service';
import { mlJobService } from '../../services/job_service';

import { EXPLORER_ACTION } from '../explorer_constants';
import { createJobs } from '../explorer_utils';

export function jobSelectionActionCreator(selectedJobIds: string[]) {
export function jobSelectionActionCreator(
mlFieldFormatService: MlFieldFormatService,
selectedJobIds: string[]
) {
return from(mlFieldFormatService.populateFormats(selectedJobIds)).pipe(
map((resp) => {
if (resp.error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,29 @@ import { AnomalyChartsStateService } from './anomaly_charts_state_service';
import { AnomalyExplorerChartsService } from '../services/anomaly_explorer_charts_service';
import { useTableSeverity } from '../components/controls/select_severity';
import { AnomalyDetectionAlertsStateService } from './alerts';

export type AnomalyExplorerContextValue =
| {
anomalyExplorerChartsService: AnomalyExplorerChartsService;
anomalyExplorerCommonStateService: AnomalyExplorerCommonStateService;
anomalyTimelineService: AnomalyTimelineService;
anomalyTimelineStateService: AnomalyTimelineStateService;
chartsStateService: AnomalyChartsStateService;
anomalyDetectionAlertsStateService: AnomalyDetectionAlertsStateService;
}
| undefined;
import { explorerServiceFactory, type ExplorerService } from './explorer_dashboard_service';

export interface AnomalyExplorerContextValue {
anomalyExplorerChartsService: AnomalyExplorerChartsService;
anomalyExplorerCommonStateService: AnomalyExplorerCommonStateService;
anomalyTimelineService: AnomalyTimelineService;
anomalyTimelineStateService: AnomalyTimelineStateService;
chartsStateService: AnomalyChartsStateService;
anomalyDetectionAlertsStateService: AnomalyDetectionAlertsStateService;
explorerService: ExplorerService;
}

/**
* Context of the Anomaly Explorer page.
*/
export const AnomalyExplorerContext = React.createContext<AnomalyExplorerContextValue>(undefined);
export const AnomalyExplorerContext = React.createContext<AnomalyExplorerContextValue | undefined>(
undefined
);

/**
* Hook for consuming {@link AnomalyExplorerContext}.
*/
export function useAnomalyExplorerContext():
| Exclude<AnomalyExplorerContextValue, undefined>
| never {
export function useAnomalyExplorerContext() {
const context = useContext(AnomalyExplorerContext);

if (context === undefined) {
Expand All @@ -59,7 +59,7 @@ export const AnomalyExplorerContextProvider: FC = ({ children }) => {

const {
services: {
mlServices: { mlApiServices },
mlServices: { mlApiServices, mlFieldFormatService },
uiSettings,
data,
},
Expand All @@ -70,10 +70,17 @@ export const AnomalyExplorerContextProvider: FC = ({ children }) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
const mlResultsService = useMemo(() => mlResultsServiceProvider(mlApiServices), []);

const [anomalyExplorerContextValue, setAnomalyExplorerContextValue] =
useState<AnomalyExplorerContextValue>(undefined);
const [anomalyExplorerContextValue, setAnomalyExplorerContextValue] = useState<
AnomalyExplorerContextValue | undefined
>(undefined);

// It might look tempting to refactor this into `useMemo()` and just return
// `anomalyExplorerContextValue`, but these services internally might call other state
// updates so using `useEffect` is the right thing to do here to not get errors
// related to React lifecycle methods.
useEffect(() => {
const explorerService = explorerServiceFactory(mlFieldFormatService);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here - mlFieldFormatService doesn't need to be pulled from context but can be instantiated in this component once (using the required services already in context) to be passed on.


const anomalyTimelineService = new AnomalyTimelineService(
timefilter,
uiSettings,
Expand Down Expand Up @@ -118,6 +125,7 @@ export const AnomalyExplorerContextProvider: FC = ({ children }) => {
anomalyTimelineStateService,
chartsStateService,
anomalyDetectionAlertsStateService,
explorerService,
});

return () => {
Expand Down
Loading