[ML] Anomaly Detection: Visualize delayed data#101236
Merged
alvarezmelissa87 merged 10 commits intoelastic:masterfrom Jun 10, 2021
Merged
[ML] Anomaly Detection: Visualize delayed data#101236alvarezmelissa87 merged 10 commits intoelastic:masterfrom
alvarezmelissa87 merged 10 commits intoelastic:masterfrom
Conversation
Contributor
|
Pinging @elastic/ml-ui (:ml) |
15 tasks
Member
There was a problem hiding this comment.
bool may not exist here so may need creating too.
Contributor
Author
There was a problem hiding this comment.
Yep - good catch 👍 Thanks!
Member
There was a problem hiding this comment.
it would be good not to use any here.
i'm not sure what the correct official query type is anymore. the esclient has QueryDslQueryContainer which fits.
Contributor
Author
There was a problem hiding this comment.
Type no longer needed as this has been moved to server side behind endpoint 👍
Member
There was a problem hiding this comment.
runtime_mappings need to be added to the query and used in the searches.
Member
There was a problem hiding this comment.
looks like this can be BucketResultsForChart rather than any
Member
There was a problem hiding this comment.
looks like this can be number[][] rather than any
Member
There was a problem hiding this comment.
i think a lot of this logic could be moved to the server side or at least out of this react component.
The advantage of moving to the server side is these searches could all happen internally and the processing of the data would not be performed in the browser
Contributor
Author
There was a problem hiding this comment.
Yeah, now that I look at it all I think it makes sense to do that - I'll move it all behind a single endpoint 👍
Especially if we want to also separately re-request the datafeed config
Contributor
Author
There was a problem hiding this comment.
Moved to server side in b201a635f08667349e20b1631023c46d1162a3b5
Member
There was a problem hiding this comment.
we have a DatafeedWithStats interface here
Contributor
Author
There was a problem hiding this comment.
This won't be needed once we fetch the datafeed object separately in the follow up. Will update then 👍
Member
There was a problem hiding this comment.
i added a couple of other comments below, with them it looks like this component doesn't need the whole datafeed at all, only the datafeed id and query delay.
it doesn't really need the state either, but instead could take a isEnabled or similar flag.
Contributor
Author
There was a problem hiding this comment.
Good suggestion - updated to only take datafeedId, queryDelay, and isEnabled flag in d40f021
Member
There was a problem hiding this comment.
Even though datafeedConfig and bucketSpan are available from the jobs list, I think it would be safer to load these again when the modal opens.
Doing so means they are up to date and the datafeedConfig isn't a DatafeedWithStats object and so doesn't need the stats to be stripped out.
It also makes the component more self contained and is how the edit job modal works.
As a side note, the ...WithStats interfaces we have should be avoided as they are artificial and cause confusion. If we need to pass a datafeed and its stats somewhere, they should be kept as separate objects.
Contributor
Author
There was a problem hiding this comment.
Good call. Updated DatafeedModal to fetch the necessary data in the component itself in d40f021
This is done in a way consistent with edit_job_flyout.js - job is fetched and datafeed_config is used from the response.
Member
There was a problem hiding this comment.
as mentioned in a previous comment, it would be better to only work with Datafeed objects if possible.
Contributor
Author
There was a problem hiding this comment.
Removed as datafeedConfig not being sent to this component anymore - d40f021
Member
There was a problem hiding this comment.
it doesn't look like unshift is needed here, push could be used.
lcawl
reviewed
Jun 4, 2021
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
b201a63 to
fbc18c2
Compare
fbc18c2 to
cf94886
Compare
| /** | ||
| * Datafeed config for searching source | ||
| */ | ||
| datafeedConfig: schema.any(), |
Member
There was a problem hiding this comment.
the datafeed doesn't need to be passed in if you have the job id, getDatafeedByJobId can be used to load the datafeed
Contributor
Author
There was a problem hiding this comment.
Only passed jobId and fetched the datafeed in the endpoint in d40f021
| try { | ||
| await ml.updateDatafeed({ | ||
| datafeedId, | ||
| datafeedConfig: { ...datafeedConfigClone, query_delay: newQueryDelay }, |
Member
There was a problem hiding this comment.
the whole datafeed doesn't need to be passed in here, only { query_delay: newQueryDelay }
Contributor
Author
There was a problem hiding this comment.
Good catch - updated in d40f021
Along with the updateDatafeed type
qn895
reviewed
Jun 8, 2021
x-pack/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/constants.ts
Show resolved
Hide resolved
.../plugins/ml/public/application/components/annotations/annotations_table/annotations_table.js
Outdated
Show resolved
Hide resolved
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
| <EuiFlexItem grow={false}> | ||
| <EuiFlexGroup justifyContent="spaceBetween"> | ||
| <EuiFlexItem grow={false}> | ||
| <EuiSelect |
Contributor
There was a problem hiding this comment.
I am not convinced we need this interval dropdown. We could just show the maximum number of points that can be rendered clearly by the chart (480? 960? - probably some multiple of 12). The user can already navigate easily with the back and forward buttons and the time picker control.
The options in the select don't seem to be optimized for longer bucket spans - here for example I have a job with a 12h bucket span, and it opens with a default of 3h. Removing the range control would remove the complexity of working out the best range of options to show depending on the bucket span, which could range from seconds to days for example.
| /> | ||
| </EuiFlexItem> | ||
| <EuiFlexItem grow={false}> | ||
| <EuiDatePicker |
Contributor
There was a problem hiding this comment.
I wonder if this date picker could do with a label, such as end time or showing data to. Without a label it needs an aria label I think for accessibility.
| import { DATAFEED_STATE } from '../../../../../../common/constants/states'; | ||
| import { CombinedJobWithStats } from '../../../../../../common/types/anomaly_detection_jobs'; | ||
| import { useToastNotificationService } from '../../../../services/toast_notification_service'; | ||
| import { ml } from '../../../../services/ml_api_service'; |
Member
There was a problem hiding this comment.
for new code it's better to use our api context rather than importing ml which uses the dependency cache
import { useMlApiContext } from '../../../../contexts/kibana';
const {
results: { getDatafeedResultChartData },
} = useMlApiContext();
| EuiToolTip, | ||
| } from '@elastic/eui'; | ||
|
|
||
| import { ml } from '../../../../services/ml_api_service'; |
Member
There was a problem hiding this comment.
same as previous comment, please use useMlApiContext
Contributor
Author
There was a problem hiding this comment.
Ah, thanks for the reminder. Updated both in 1df1594
| body: { desc: true, start: String(start), end: String(end), page: { from: 0, size: 1000 } }, | ||
| }); | ||
|
|
||
| const bucketResults = get(bucketResp, ['body', 'buckets'], []); |
Member
There was a problem hiding this comment.
IMO we shouldn't be using lodash for basic things like this, I appreciate this was copied from other functions in this file, but ideally lodash will be replaced in a future refactor.
const bucketResults = bucketResp?.body?.buckets ?? [] would be better.
As well as the uses of get below
Contributor
Author
There was a problem hiding this comment.
Yep, agreed. Kept it in initially for consistency but agree moving away from it is better 👍 Updated in 1df1594
| }); | ||
|
|
||
| const bucketResults = get(bucketResp, ['body', 'buckets'], []); | ||
| each(bucketResults, (dataForTime) => { |
Member
There was a problem hiding this comment.
Similarly, I don't see the benefit of using lodash's each here, bucketResults.forEach would be better IMO
Contributor
Author
There was a problem hiding this comment.
Removed all lodash function usage in 1df1594
| try { | ||
| const { getDatafeedResultsChartData } = resultsServiceProvider(mlClient, client); | ||
| const resp = await getDatafeedResultsChartData( | ||
| request.body as DatafeedResultsChartDataParams |
Member
There was a problem hiding this comment.
this cast to DatafeedResultsChartDataParams is unnecessary
Comment on lines
+626
to
+630
| const datafeedConfig = await getDatafeedByJobId(jobId); | ||
|
|
||
| const { | ||
| body: { jobs }, | ||
| } = await mlClient.getJobs({ job_id: jobId }); |
Member
There was a problem hiding this comment.
real nit, but getDatafeedByJobId and getJobs could be called in parallel.
Member
There was a problem hiding this comment.
just in case this comment was lost in a different change, I still think putting them in a Promise.all is worth it. But it's still only a nitpick, so please ignore if you want.
| const { | ||
| body: { jobs }, | ||
| } = await mlClient.getJobs({ job_id: jobId }); | ||
| const jobConfig = jobs[0]; |
Member
There was a problem hiding this comment.
jobs[0] could be undefined here if this endpoint is called with a wildcard in the jobId
Testing this throws a kibana internal server error.
Contributor
Author
There was a problem hiding this comment.
Good catch - updated to handle the job not being found in 1df1594
Contributor
peteharverson
left a comment
There was a problem hiding this comment.
Left a few extra comments on the styling of the chart.
Overall looks good. In the follow-up would be good to get the 'View datafeed' action in the top level menu potentially, and/or from the Datafeed tab in the expanded row.
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
| showOverlappingTicks | ||
| tickFormat={dateFormatter} | ||
| title={i18n.translate('xpack.ml.jobsList.datafeedModal.xAxisTitle', { | ||
| defaultMessage: 'Bucket span ({bucketSpan})', |
Contributor
There was a problem hiding this comment.
I'd not bother with a title for x axis, and move the bucket span to the top of modal, somewhere near the job ID perhaps?
Contributor
Author
There was a problem hiding this comment.
Keeping it in for now as I think it might be confusing for the user to remove it but will look at other options in the follow up. 👍
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/ml/public/application/jobs/jobs_list/components/datafeed_modal/datafeed_modal.tsx
Outdated
Show resolved
Hide resolved
qn895
reviewed
Jun 9, 2021
| }, | ||
| }); | ||
|
|
||
| if (this.state.jobId && this.props.jobs[0].analysis_config.bucket_span) { |
Member
There was a problem hiding this comment.
Seems like we keep using this.props.jobs[0] throughout the file. Can we make a constant to access this job?
Contributor
Author
|
@elasticmachine merge upstream |
qn895
reviewed
Jun 9, 2021
|
|
||
| import { i18n } from '@kbn/i18n'; | ||
|
|
||
| export const getIntervalOptions = (bucketSpan: string) => { |
Member
There was a problem hiding this comment.
I think it would be good to add some unit testing for this utility function in a follow up
Contributor
Author
|
Comments have been addressed - thanks for all the testing 🙏 |
qn895
approved these changes
Jun 9, 2021
Member
|
Code LGTM 🎉 |
jgowdyelastic
approved these changes
Jun 10, 2021
Member
jgowdyelastic
left a comment
There was a problem hiding this comment.
Added a comment about making some calls in parallel, but generally LGTM
| } from './constants'; | ||
| import { loadFullJob } from '../utils'; | ||
|
|
||
| const dateFormatter = timeFormatter('MM-DD HH:mm'); |
Contributor
There was a problem hiding this comment.
This should go to the ss level where the bucket span is less than 1 minute.
peteharverson
approved these changes
Jun 10, 2021
Contributor
peteharverson
left a comment
There was a problem hiding this comment.
Tested latest edits and LGTM. Left one extra suggestion about the tooltip time format, but that can be added to the follow-up.
alvarezmelissa87
added a commit
that referenced
this pull request
Jun 10, 2021
…01912) * wip: add datafeed modal for chart * Add arrows to navigate through time in the chart * ensure runtime_mapping and indices_options in search query * move chart data fetching behind single server endpoint * remove success check as it is not returned in result * load necessary modal data in modal * remove extra legend and add text to action icons * remove unused endpoint and types * handle job not found and fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Contributor
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/ml/alert_flyout·ts.ML app anomaly detection alert overview page alert flyout controls can create an anomaly detection alertStandard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/ml/alert_flyout·ts.ML app anomaly detection alert overview page alert flyout controls can create an anomaly detection alertStandard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indicesStandard OutStack TraceMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related meta issue: #99510
This PR adds a modal that is accessible through a
View datafeedaction from the expanded rowAnnotationstable.The modal charts the job's source data from the datafeed along with the bucket results to compare record counts per bucket span. This will allow the user to easily visualize delayed data. The modal allows the user to update the
query_delay.Checklist
Delete any items that are not applicable to this PR.