-
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] Add embedded map to geo_point fields for Data Visualizer #88880
Conversation
Pinging @elastic/ml-ui (:ml) |
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.
Really cool to have the maps in the data visualizer row expansions, great addition!
Functional tests in the file data viz LGTM, just one small suggestion.
I've noticed that the expanded row content is different in file data viz compared to the index data viz after import:
File data viz before import
This shows document stats, top values and the map.
This shows document stats, examples and the map.
Is this difference intended?
And related to that: The service method assertGeoPointFieldContents
will currently only work for the file data viz (because of that difference). And it would be good to also have functional tests for maps in the index data viz, so we should either make both row contents the same or have separate service methods to validate.
x-pack/test/functional/apps/ml/data_visualizer/file_data_visualizer.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/services/ml_api_service/jobs.ts
Outdated
Show resolved
Hide resolved
updateIndexPatternSearchLayer(); | ||
}, [indexPattern, config.fieldName, combinedQuery]); | ||
|
||
if (stats?.examples === undefined) return null; |
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 don't think this is related to your PR, but for this filebeat-apache-2019.01.30
data set, no examples are displayed for the traefik.access.geoip.location
field.
I remember seeing something like this in the card based version, which was related to the query we were using to get the examples - https://github.com/elastic/kibana/blob/7.9/x-pack/plugins/ml/public/application/datavisualizer/index_based/components/field_data_card/content_types/geo_point_content.tsx
If the query to get the examples returns no docs, we should probably show some sort of message in that cell saying something like No examples were obtained for this field
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 added this message here f537b8c
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.
Showing the No examples were obtained for this field
is good here, but I think we can actually improve the search used to obtain the examples in ml/server/models/data_visualizer/data_visualizer.ts
getFieldExamples
too. I think we can edit that search to use the fields parameter in place of the current _source
parameter. I gave this a quick try and examples now show up for the traefik.access.geoip.location
field above:
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 here d44517f
The example lists are now using fields
API, which will look different since the API auto detects and auto formats.
@pheyos regarding #88880 (review), I think we have to show examples for the index based view as geo fields don't support aggregations. Showing the examples list for the file based view would work for me, if we want to use the same component for both. I guess the radius of the circles on the map gives an indication of the relative frequencies (which is what the top values list provides too). |
I agree @peteharverson, showing the examples list in both views sounds good. |
This reverts commit 3a50b3c
x-pack/plugins/ml/public/application/components/ml_embedded_map/_ml_embedded_map.scss
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/components/ml_embedded_map/ml_embedded_map.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/components/ml_embedded_map/ml_embedded_map.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/components/ml_embedded_map/ml_embedded_map.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/components/ml_embedded_map/ml_embedded_map.tsx
Outdated
Show resolved
Hide resolved
...ns/ml/public/application/datavisualizer/index_based/components/expanded_row/expanded_row.tsx
Outdated
Show resolved
Hide resolved
.../public/application/datavisualizer/index_based/components/expanded_row/geo_point_content.tsx
Outdated
Show resolved
Hide resolved
...c/application/datavisualizer/index_based/components/field_data_row/top_values/top_values.tsx
Outdated
Show resolved
Hide resolved
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 to see that file data viz and index data viz are showing the same components for the geo row expansion now 🎉
One suggestion and one comment about the failing tests:
x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts
Outdated
Show resolved
Hide resolved
[ML] Update shard sizes
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.
Functional tests LGTM 🎉
Just one question around variable naming.
x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts
Outdated
Show resolved
Hide resolved
stats.examples.push(example); | ||
if (stats.examples.length === maxExamples) { | ||
break; | ||
const doc: any = get(hits[i].fields, field); |
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.
did you consider this instead of any
?
const doc: any = get(hits[i].fields, field); | |
const doc: object[] | undefined = get(hits[i].fields, field); |
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 edits. LGTM. A great addition!
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
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
This PR is part of #86387. It adds embedded maps to the geo_point fields for the data visualizer (both the file based and index based). Changes include:
<MlEmbeddComponent/>
that can be consumed in other places inside Mlgeo_point
fieldsFuture Todos
Checklist
Delete any items that are not applicable to this PR.