[ML] Maps integration: adds influencers to tooltip in anomalies layer #126007

alvarezmelissa87 · 2022-02-17T23:30:09Z

Summary

Related meta issue: #123492

Adds influencers to the tooltip for each point on the map - if influencers are present.
Limits to 3 influencers and 3 influencer values to prevent tooltip from becoming too big.

Checklist

Delete any items that are not applicable to this PR.

Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

elasticmachine · 2022-02-17T23:30:11Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2022-02-17T23:30:11Z

Pinging @elastic/kibana-gis (Team:Geo)

qn895 · 2022-02-28T18:24:42Z

x-pack/plugins/ml/public/maps/util.ts

+const INFLUENCER_LIMIT = 3;
+const INFLUENCER_MAX_VALUES = 3;
+
+function getInfluencersHtmlString(


nit: Would be good to add some unit tests for this utility function

Added in 1dd5976

qn895 · 2022-02-28T18:25:28Z

x-pack/plugins/ml/public/maps/util.ts

+    }
+  }
+
+  return htmlString;


If influencers.length == 0, would this htmlString be '<ul>' which is invalid html 🤔 ?

Good call - checked for influencer length before entering the function and also moved closing tag out of the loop in 1dd5976

qn895 · 2022-02-28T18:27:45Z

Pretty sure it's not related to this PR but I wasn't sure if the tooltip popups being persisted is intentional here

Screen.Recording.2022-02-28.at.12.19.59.mov

qn895 · 2022-02-28T18:34:15Z

x-pack/plugins/ml/public/maps/util.ts

+
+    htmlString += `<li>${influencer_field_name}: ${fieldValuesString}</li>`;
+
+    // Only show top two influencers


super duper nit: since INFLUENCER_LIMIT is a constant and can potentially be changed in the future, I think this comment should not mention "two" specifically.

Good point - removed in 1dd5976

nreese · 2022-02-28T19:59:14Z

Pretty sure it's not related to this PR but I wasn't sure if the tooltip popups being persisted is intentional here

This is expected behavior. Tooltips stay opened when clicked. Tooltips auto-hide with mouse over. https://www.elastic.co/guide/en/kibana/current/vector-tooltip.html#maps-vector-tooltip-locking

qn895 · 2022-02-28T23:42:46Z

Latest changes LGTM 🎉

kibana-ci · 2022-03-01T01:54:52Z

💚 Build Succeeded

Metrics [docs]

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.5MB	3.5MB	+521.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`ml`	98	99	+1

Total ESLint disabled count

id	before	after	diff
`ml`	100	101	+1

History

💚 Build #26678 succeeded 1dd5976
💚 Build #26597 succeeded ba81ef3
💚 Build #26026 succeeded 19a90156fb6de2dd3e8dcd2e4d49e38f78141ff2
💚 Build #25709 succeeded 837395949729cf8b780cfabc73d711d2b2e10f8b
💚 Build #25140 succeeded fbc3ff2b2a80f114e1795b5bb59edaaf9ef7af9f

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

cc @alvarezmelissa87

walterra

Overall code LGTM. Just a general question because I don't know the consuming code: Can you explain why we need to create a raw HTML string instead of passing on React/TSX?

thomasneirynck · 2022-03-01T19:31:54Z

hi @walterra

wrt #126007 (review)

Can you explain why we need to create a raw HTML string instead of passing on React/TSX?

It is the Maps-plugin API that requires the tooltip-value to be a html-string. The reason for that is that Kibana-field-formatted values (as defined in the Data View in Stack Management), return actual HTML-strings that need to be displayed as is. This restriction bled through to the interfaces Maps exposes and that AnomalySourceTooltipProperty needs to implement.

See where this is being called and more details:

kibana/x-pack/plugins/maps/public/connected_components/mb_map/tooltip_control/features_tooltip/feature_properties.tsx

Lines 341 to 350 in 9e5daa5

    
                     <td 
        
                       className="eui-textOverflowWrap" 
        
                       /* 
        
                        * Justification for dangerouslySetInnerHTML: 
        
                        * Property value contains value generated by Field formatter 
        
                        * Since these formatters produce raw HTML, this component needs to be able to render them as-is, relying 
        
                        * on the field formatter to only produce safe HTML. 
        
                        */ 
        
                       dangerouslySetInnerHTML={{ __html: tooltipProperty.getHtmlDisplayValue() }} // eslint-disable-line react/no-danger 
        
                     />

imho, this is an issue mainly on the MapsPlugin API-side, and needs to be resolved there first. I'd agree that the clean solution would be that external-implementers would be able to return React-dom.

I'll create a follow-up ticket for Maps.

alvarezmelissa87 added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation :ml release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Feb 17, 2022

alvarezmelissa87 self-assigned this Feb 17, 2022

alvarezmelissa87 requested a review from a team as a code owner February 17, 2022 23:30

alvarezmelissa87 mentioned this pull request Feb 17, 2022

[Maps][ML] Integration meta issue #123492

Closed

8 tasks

alvarezmelissa87 changed the title ~~[ML] Maps integration: adds influencers to tooltip in anomalies layer~~ WIP [ML] Maps integration: adds influencers to tooltip in anomalies layer Feb 17, 2022

alvarezmelissa87 force-pushed the ml-maps-tooltip branch from fbc3ff2 to 8373959 Compare February 22, 2022 21:29

alvarezmelissa87 changed the title ~~WIP [ML] Maps integration: adds influencers to tooltip in anomalies layer~~ [ML] Maps integration: adds influencers to tooltip in anomalies layer Feb 23, 2022

alvarezmelissa87 requested review from peteharverson, walterra and qn895 February 23, 2022 19:43

alvarezmelissa87 added 2 commits February 28, 2022 10:46

adds influencers to tooltip

c03b75b

do not add to tooltip if already partition field

ba81ef3

alvarezmelissa87 force-pushed the ml-maps-tooltip branch from 19a9015 to ba81ef3 Compare February 28, 2022 17:46

qn895 reviewed Feb 28, 2022

View reviewed changes

adds unit test for influencer string function

1dd5976

qn895 approved these changes Feb 28, 2022

View reviewed changes

add unit test for getResultsForJobId

15bc49c

walterra approved these changes Mar 1, 2022

View reviewed changes

alvarezmelissa87 merged commit 058ad18 into elastic:main Mar 1, 2022

alvarezmelissa87 deleted the ml-maps-tooltip branch March 1, 2022 17:29

alvarezmelissa87 added the backport:skip This commit does not require backporting label Mar 1, 2022

thomasneirynck mentioned this pull request Mar 1, 2022

[Maps] Allow implementers of ITooltipProperty to return React-DOM for the tooltip-value #126631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Maps integration: adds influencers to tooltip in anomalies layer #126007

[ML] Maps integration: adds influencers to tooltip in anomalies layer #126007

alvarezmelissa87 commented Feb 17, 2022 •

edited

Loading

elasticmachine commented Feb 17, 2022

elasticmachine commented Feb 17, 2022

qn895 Feb 28, 2022

alvarezmelissa87 Feb 28, 2022

qn895 Feb 28, 2022

alvarezmelissa87 Feb 28, 2022

qn895 commented Feb 28, 2022

qn895 Feb 28, 2022

alvarezmelissa87 Feb 28, 2022

nreese commented Feb 28, 2022

qn895 commented Feb 28, 2022

kibana-ci commented Mar 1, 2022

ESLint disabled line counts

Total ESLint disabled count

walterra left a comment

thomasneirynck commented Mar 1, 2022 •

edited

Loading


		htmlString += `<li>${influencer_field_name}: ${fieldValuesString}</li>`;

		// Only show top two influencers

[ML] Maps integration: adds influencers to tooltip in anomalies layer #126007

[ML] Maps integration: adds influencers to tooltip in anomalies layer #126007

Conversation

alvarezmelissa87 commented Feb 17, 2022 • edited Loading

Summary

Checklist

elasticmachine commented Feb 17, 2022

elasticmachine commented Feb 17, 2022

qn895 Feb 28, 2022

Choose a reason for hiding this comment

alvarezmelissa87 Feb 28, 2022

Choose a reason for hiding this comment

qn895 Feb 28, 2022

Choose a reason for hiding this comment

alvarezmelissa87 Feb 28, 2022

Choose a reason for hiding this comment

qn895 commented Feb 28, 2022

qn895 Feb 28, 2022

Choose a reason for hiding this comment

alvarezmelissa87 Feb 28, 2022

Choose a reason for hiding this comment

nreese commented Feb 28, 2022

qn895 commented Feb 28, 2022

kibana-ci commented Mar 1, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

walterra left a comment

Choose a reason for hiding this comment

thomasneirynck commented Mar 1, 2022 • edited Loading

alvarezmelissa87 commented Feb 17, 2022 •

edited

Loading

thomasneirynck commented Mar 1, 2022 •

edited

Loading