-
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] Improve messaging and support for datafeed using aggregated and scripted fields #84594
[ML] Improve messaging and support for datafeed using aggregated and scripted fields #84594
Conversation
…pported-aggs-script
…pported-aggs-script
Pinging @elastic/ml-ui (:ml) |
'xpack.ml.timeSeriesJob.varyingBucketSpanAggregationInterval', | ||
{ | ||
defaultMessage: | ||
'bucket span and aggregation interval is not the same for datafeed with aggregation fields', |
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 this could be clarified a bit. For example:
'the datafeed has aggregation fields and the aggregation interval is not the same as the bucket span',
or:
'bucket span and aggregation interval are not the same for a datafeed with aggregation fields',
I prefer the first suggestion.
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.
My vote goes to the first suggestion from @lcawl
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 02a579b
'xpack.ml.timeSeriesJob.varyingBucketSpanAggregationInterval', | ||
{ | ||
defaultMessage: | ||
'bucket span and aggregation interval is not the same for datafeed with aggregation fields', |
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.
My vote goes to the first suggestion from @lcawl
x-pack/plugins/ml/public/application/components/custom_hooks/use_create_ad_links.ts
Show resolved
Hide resolved
...rame_analytics/pages/analytics_creation/components/back_to_list_panel/back_to_list_panel.tsx
Show resolved
Hide resolved
} | ||
|
||
// if aggregation interval is different from bucket span | ||
const datetimeBucket = aggs[aggBucketsName].date_histogram; |
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.
For this use case, I think we should enable access to the Single Metric Viewer, and show the charts in the Anomaly Explorer, but display a Toast / Callout which explains that the numbers on the chart may differ from the values reported for the anomaly. For example, this chart provides a lot of value:
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.
Update - as discussed, we should just show the charts in this case, with no Toast / Callout about the differing bucket span and aggregation interval, as otherwise they would show up for jobs created in the Single Metric wizard, where we set the agg interval to 10% of the bucket span. We can address that edge case in a future PR, to show a toast warning only if the function
used in the detector is different to that used in the datafeed aggregation.
.../ml/public/application/timeseriesexplorer/timeseriesexplorer_utils/validate_job_selection.ts
Show resolved
Hide resolved
// Returns a flag to indicate whether the job is suitable for viewing | ||
// in the Time Series dashboard. | ||
export function isTimeSeriesViewJob(job: CombinedJob): boolean { | ||
export function isTimeSeriesViewableJob(job: CombinedJob): boolean { |
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.
Removed here 15cc5ed
jobsErrorMessage[record.job_id] = i18n.translate( | ||
'xpack.ml.timeSeriesJob.sourceDataModelPlotNotChartableMessage', | ||
{ | ||
defaultMessage: 'both source data and model plot not chartable for this detector', |
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 this needs are
inserted, source data and model plot are not chartable
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 ce964f5
<EuiCallOut color={'warning'} size="s"> | ||
<FormattedMessage | ||
id="xpack.ml.explorerCharts.errorCallOutMessage" | ||
defaultMessage="You can't view anomaly records for {jobs} because {reason}." |
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.
Swap the word records
for charts
.
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 ce964f5
…pported-aggs-script
…egated/scripted fields
As discussed I have updated a logic to be less strict so that it will not necessarily disable when there's nested term aggregations or when the bucket span != aggregation interval. This is so examples like below will still show up, without callouts, despite the anomaly markers not plotting correctly:
|
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.
Latest changes LGTM, great you added some functional tests too! 👍
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 and LGTM!
x-pack/test/functional/apps/ml/anomaly_detection/aggregated_scripted_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection/aggregated_scripted_job.ts
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection/aggregated_scripted_job.ts
Outdated
Show resolved
Hide resolved
Started flaky test suite runner... |
x-pack/test/functional/apps/ml/anomaly_detection/aggregated_scripted_job.ts
Outdated
Show resolved
Hide resolved
…obSelectionNotContains
Started flaky test suite runner... |
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, thanks for adding them as part of this PR! 🎉
Just one small nit after the latest change, but feel free to merge as is if you want.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Summary
This PR adds better messaging for unsupported configurations and improved support for aggregated and scripted fields. Changes include:
Add ability to create and view job when datafeed uses something other than
buckets
in the datafeed's aggregation (e.g. an arbitrary name like 'my_buckets'). Previously, this is only supported via the API. This PR makes it so that the fields are listed correctly in the job creation wizard and makes the charts plot correctly.Ability to plot Anomaly Explorer charts for jobs with aggregated or scripted fields with model plot data in cases if source chart is not chartable.
Checklist
Delete any items that are not applicable to this PR.
For maintainers