-
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] Anomaly Detection: Fix anomaly marker click in Single Metric Viewer embeddable #176788
Changes from 10 commits
c07d8d7
85dc3f2
c84fedc
9ff3807
4abe2a9
0ce7c8a
5e479f4
1512c2f
2fcc8b6
02b1dfc
ac060ae
5508ae2
70ab770
ea30406
ad49b54
42e89c1
f57c08d
bccd244
1a4f9b6
3ee830d
9333edc
3f40ece
bcda568
d4e6b66
cef71e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned after James's comment below - the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
|
@@ -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( | ||
|
@@ -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) { | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same note here for either instantiating 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 70ab770. |
||
let fetchedDataView: DataView | undefined; | ||
|
||
try { | ||
|
@@ -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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,7 @@ export const Controls: FC<Props> = React.memo( | |
|
||
const { | ||
services: { | ||
data, | ||
share, | ||
application: { navigateToUrl, capabilities }, | ||
}, | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 70ab770. |
||
|
||
if (dataViewId !== null) { | ||
const path = await mlLocator.getUrl({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -59,7 +59,7 @@ export const AnomalyExplorerContextProvider: FC = ({ children }) => { | |
|
||
const { | ||
services: { | ||
mlServices: { mlApiServices }, | ||
mlServices: { mlApiServices, mlFieldFormatService }, | ||
uiSettings, | ||
data, | ||
}, | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same note here - |
||
|
||
const anomalyTimelineService = new AnomalyTimelineService( | ||
timefilter, | ||
uiSettings, | ||
|
@@ -118,6 +125,7 @@ export const AnomalyExplorerContextProvider: FC = ({ children }) => { | |
anomalyTimelineStateService, | ||
chartsStateService, | ||
anomalyDetectionAlertsStateService, | ||
explorerService, | ||
}); | ||
|
||
return () => { | ||
|
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.
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
anddataViews
in context, I believe.fieldFormatServiceFactory
requiresmlApiServices
andmlIndexUtils
- 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.
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.
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.
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 tend to agree that it would be better to keep
mlIndexUtils
andmlFieldFormatService
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.
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 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 viauseMlIndexUtils()
.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):
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.
Does this still require a code change?
mlIndexUtils
is still being added to the context.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.
Oh yes, overlooked that one, thanks! Fixed in bccd244.