[ML] Extends support for anomaly charts when model plot is enabled#34079
[ML] Extends support for anomaly charts when model plot is enabled#34079peteharverson merged 2 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/ml-ui |
💚 Build Succeeded |
jgowdyelastic
left a comment
There was a problem hiding this comment.
Added a couple of comments, but generally it all LGTM
| // of objects in the form { fieldName: airline, fieldValue: AAL, fieldType: partition } | ||
| export function getEntityFieldList(record) { | ||
| const entityFields = []; | ||
| if (_.get(record, 'partition_field_name') !== undefined) { |
There was a problem hiding this comment.
Using _.get for these checks unnecessary IMO.
this check could just be record.partition_field_name !== undefined.
Edit:
I just noticed this code was moved from somewhere else. so ignore this if you don't want the code churn.
At some point we should look to review _.get usage as it was flagged by kibana as potentially dangerous due to prototype pollution. If we ever use it to get a user defined key, that should change.
I'd also personally like to change any situations like the one above where it's being used to get a single, non-nested key, as it's unnecessary and adds complexity.
There was a problem hiding this comment.
Agreed. Seeing as this is a new function, I'll switch the _.get to e.g. record.partition_field_name here.
| // plus varp and info_content functions. | ||
| isModelPlotChartable = (mlFunctionToESAggregation(functionName) !== null) || | ||
| (['varp', 'high_varp', 'low_varp', 'info_content', | ||
| 'high_info_content', 'low_info_content'].indexOf(functionName) > -1); |
There was a problem hiding this comment.
.includes rather than .indexOf could be used here.
Doesn't gain anything really, but i think it reads a bit better.
| $scope.hasResults = false; | ||
| $scope.loading = false; | ||
| $scope.dataNotChartable = true; | ||
| $scope.$applyAsync(); |
There was a problem hiding this comment.
it doesn't look like a promise is being used in this code, is this $applyAsync needed?
There was a problem hiding this comment.
Yes this $applyAsync is needed. When you are viewing an entity which triggers this block, if you then hard refresh the page, the $applyAsync is needed to get the view to update correctly and show the correct error message on initial load.
💚 Build Succeeded |
…lastic#34079) * [ML] Extends support for anomaly charts when model plot is enabled * [ML] Edits to util functions following review
Summary
Previously anomaly charts were only shown in the Anomaly Explorer, and a detector was only visible in the Single Metric Viewer, if the detector
function_descriptionwas one ofmean,min,max,sum,count,distinct_count,median, orrareand the detector did not use a scripted field that was defined in the datafeed configuration.If model plot has been enabled, this PR extends support for charts in the Anomaly Explorer, and the detector is visible in the Single Metric Viewer, to detectors where:
field_nameof the detector is a script field defined in the datafeed configurationfunction_descriptionof the detector isvarpfunction_descriptionof the detector isinfo_contentmlcategorye.g. of a chart in the Anomaly Explorer for an
info_contentjob where model plot has been enabled:Checklist
For maintainers
See #22922.
Fixes #32124.