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.

Sign up for GitHub

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

Jump to bottom

[ML][Maps] Adds link to maps in charts section of Anomaly Explorer #128697

Merged
merged 7 commits into from
Mar 30, 2022
Merged

[ML][Maps] Adds link to maps in charts section of Anomaly Explorer #128697

merged 7 commits into from
Mar 30, 2022

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Mar 28, 2022 •
edited
Loading

Summary

Related meta issue: #123492

This PR adds a link in the anomaly explorer embedded maps to the Maps plugin for displaying the anomalies for that job id.

image

Links to:

image

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner March 28, 2022 23:19
@alvarezmelissa87 alvarezmelissa87 self-assigned this Mar 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Mar 28, 2022
Closed
8 tasks
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner March 29, 2022 00:10
nreese
nreese reviewed Mar 29, 2022
View reviewed changes
x-pack/plugins/maps/public/index.ts
@@ -17,7 +17,8 @@ export const plugin: PluginInitializer<MapsPluginSetup, MapsPluginStart> = (
return new MapsPlugin(initContext);
};

export { MAP_SAVED_OBJECT_TYPE } from '../common/constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of exporting APP_ID in maps/public/index, an ML just import from maps/common. You will have to add APP_ID to maps/common/index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - moved to export from /common in 3bfae94

peteharverson
peteharverson reviewed Mar 29, 2022
View reviewed changes
x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_charts_container.js
@@ -191,6 +265,23 @@ function ExplorerChartContainer({
</EuiButtonEmpty>
</EuiToolTip>
)}
{chartType === CHART_TYPE.GEO_MAP && mapsLink ? (
<EuiToolTip position="top" content={openInMapsPluginMessage}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed for this PR, but would be great to add a View in Maps item to the Actions menu in the anomalies table, similar to the View series link for time series anomalies which is available for the anomaly charts and the anomalies table.

peteharverson
peteharverson reviewed Mar 29, 2022
View reviewed changes
x-pack/plugins/ml/public/maps/anomaly_source.tsx

if (meta?.areResultsTrimmed) {
return {
tooltipContent: i18n.translate('xpack.ml.maps.resultsTrimmedMsg', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this code, but is it possible to only add the Influencers section of the point tooltip if they are not already shown as a partition field. For example here the only influencer I have configured is vehicle.id so the empty Influencers section is redundant:

image

nreese
nreese approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM, changes to anomaly_source.tsx areResultsTrimmed LGTM
code review.

@qn895
Copy link
Member

qn895 commented Mar 29, 2022

Tested latest changes and LGTM but might be worth testing it some more with where partition fields contain special characters

peteharverson
peteharverson approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a 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

@alvarezmelissa87
Copy link
Contributor Author

alvarezmelissa87 commented Mar 29, 2022 •
edited
Loading

Tested latest changes and LGTM but might be worth testing it some more with where partition fields contain special characters

Added tests for the query creation and added use of escapeKuery from @kbn/es-query service to ensure special characters are escaped in fd5c131
cc @peteharverson, @qn895 if you still wanted to give it a final test.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
maps 219 221 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.3MB 3.3MB +2.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 69.3KB 69.4KB +90.0B
Unknown metric groups

API count

id before after diff
maps 220 222 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @alvarezmelissa87

@alvarezmelissa87 alvarezmelissa87 merged commit 4f070b3 into elastic:main Mar 30, 2022
@alvarezmelissa87 alvarezmelissa87 deleted the ml-maps-external-link branch March 30, 2022 07:04
@thomasneirynck thomasneirynck mentioned this pull request Mar 31, 2022
Closed
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 128697 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 1, 2022
@peteharverson peteharverson added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Apr 1, 2022
@peteharverson peteharverson added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels Apr 13, 2022
@lcawl lcawl changed the title [ML][Maps] Anomaly Detection: Add link to maps in charts section of Anomaly Explorer [ML][Maps] Adds link to maps in charts section of Anomaly Explorer Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qn895 qn895 qn895 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@nreese nreese nreese approved these changes

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
backport:skip This commit does not require backporting :ml release_note:feature Makes this part of the condensed release notes v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alvarezmelissa87 @elasticmachine @qn895 @kibana-ci @kibanamachine @nreese @peteharverson