-
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: add anomalies map to explorer for jobs with 'lat_long' function #88416
[ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function #88416
Conversation
Pinging @elastic/ml-ui (:ml) |
...ck/plugins/ml/public/application/explorer/components/explorer_map_container/embedded_map.tsx
Outdated
Show resolved
Hide resolved
.../ml/public/application/explorer/components/explorer_map_container/explorer_map_container.tsx
Outdated
Show resolved
Hide resolved
Love the embedded map in the Anomaly Explorer! Some thoughts:
|
This looks great! Agree with Pete's thought: What if we make this "just" another chart type of the charts when you drill down and click on anomalies (e.g. regular line chart, rare chart, population chart, map). For those charts we don't show |
|
||
return ( | ||
<div | ||
className={'mlFieldDataCard__geoContent'} |
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 think we can remove this className here
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.
Removed 👍
398e64e
to
90141a2
Compare
This has been updated and is ready for another look when you get a chance. cc @walterra, @peteharverson 🙏 |
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.
Great progress! Added some more comments.
// import { AnomalyRecordDoc } from '../../../../common/types/seriesConfig.chartData'; | ||
|
||
interface Props { | ||
seriesConfig?: any; // object; |
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.
Since all other charts code is still plain JS, agree to use the any
here in this PR. I added an item to this charts meta-issue that we should migrate to TS #21163.
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 a9b5c32580d12c8a582b382049631e9709a7230a
if (!embeddablePlugin) { | ||
throw new Error('Embeddable start plugin not found'); | ||
} | ||
if (!mapsPlugin) { | ||
throw new Error('Maps start plugin not found'); | ||
} | ||
|
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.
If I remember correctly, for the file/data visualizer the maps plugin is defined as an optional plugin. Are there plans for a more graceful fallback in the UI if the plugin is not available?
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.
You're correct - will update to optional as it should be and show a toast with a message that the plugin is disabled but let the anomaly explorer load normally otherwise 👍
throw new Error('Maps start plugin not found'); | ||
} | ||
|
||
const factory: any = embeddablePlugin.getEmbeddableFactory(MAP_SAVED_OBJECT_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.
Looks like embeddablePlugin
is fully typed, can we get rid of the 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.
Removed in a9b5c32580d12c8a582b382049631e9709a7230a
const factory: any = embeddablePlugin.getEmbeddableFactory(MAP_SAVED_OBJECT_TYPE); | ||
|
||
const input: MapEmbeddableInput = { | ||
id: uuid.v4(), |
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.
Based on other EUI examples, we use EUI's htmlIdGenerator
is other places like this:
import { htmlIdGenerator } from '@elastic/eui';
const id = useMemo(() => htmlIdGenerator()(), []);
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 478a75725eb997b45a1c62becae5c72bb4d9a1c2
if (!factory) { | ||
throw new Error('Map embeddable not found.'); | ||
} | ||
const embeddableObject: any = await factory.create({ |
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.
Would be great to get rid of that 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.
Updated in 478a75725eb997b45a1c62becae5c72bb4d9a1c2
> | ||
<div | ||
data-test-subj="xpack.ml.explorer.embeddedMapPanel" | ||
className="embPanel__content" |
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.
Is this class necessary? I don't see any other reference to it in the ML plugin anywhere else.
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.
Removed in 478a75725eb997b45a1c62becae5c72bb4d9a1c2
style={{ | ||
width: '100%', | ||
height: '100%', | ||
display: 'flex', | ||
flex: '1 1 100%', | ||
zIndex: 1, | ||
minHeight: 0, // Absolute must for Firefox to scroll contents | ||
}} |
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.
Can we move this to a custom class in an SCSS file?
.catch((error) => { | ||
console.error(error); | ||
}); | ||
if (!isGeoMap) { |
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.
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.
Should be fixed by 478a75725eb997b45a1c62becae5c72bb4d9a1c2
x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_chart_embedded_map.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_chart_embedded_map.tsx
Outdated
Show resolved
Hide resolved
Looks like there is an issue with display of the detector function in the chart info tooltip for Thanks for catching that, @peteharverson - fixed in 478a75725eb997b45a1c62becae5c72bb4d9a1c2 |
33a7908
to
dff81a9
Compare
3737edb
to
ea9414a
Compare
x-pack/plugins/ml/public/application/explorer/explorer_charts/map_config.ts
Show resolved
Hide resolved
)} | ||
</MlTooltipComponent> | ||
); | ||
if (chartType === CHART_TYPE.SINGLE_METRIC) { |
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.
LGTM 🎉 |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
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
…lat_long' function (#88416) (#89738) * wip: create embedded map component for explorer * add embeddedMap component to explorer * use geo_results * remove charts callout when map is shown * add translation, round geo coordinates * create GEO_MAP chart type and move embedded map to charts area * remove embedded map that is no longer used * fix type and fail silently if plugin not available * fix multiple type of jobs charts view * fix tooltip function and remove single viewer link for latlong * ensure diff types of jobs show correct charts. fix jest test * show errorCallout if maps not enabled and is lat_long job * use shared MlEmbeddedMapComponent in explorer * ensure latLong jobs not viewable in single metric viewer * update jest test
Summary
Different types of jobs selected:
When maps or embeddable plugin are not enabled we show a message to the user and only show charts that can be shown:
NOTES
Checklist
Delete any items that are not applicable to this PR.