Adding flyout to create detector from existing monitor through Monitor Details page #180
Conversation
| ) { | ||
| calloutType = 'valid'; | ||
| } | ||
| for (let [key, value] of Object.entries(context.suggestedChanges)) { |
There was a problem hiding this comment.
it seems that you're looping through all of the suggested changes, and would overwrite calloutType if there was multiple suggested changes (one for maxInterval, one for filterQueryTooSparse). Can you confirm if UX has any thoughts on showing multiple callouts if this is the case?
| if ( | ||
| Object.keys(context.failures).length == 0 && | ||
| Object.keys(context.suggestedChanges).length == 1 | ||
| ) { | ||
| calloutType = 'valid'; | ||
| } |
There was a problem hiding this comment.
This should be ran outside of the for loop, and if true, never execute the for loop. Also, why would there be any suggested changes if the callout is valid?
There was a problem hiding this comment.
This is used for the case where the suggested change is a new detector interval (a specific number and not the message that the max was reached) so at that case I just update the configurations and the callout is hence valid. I'll try to make this logic cleaner in my code
There was a problem hiding this comment.
Got it, yeah. Sounds like a specific case that could be outside of this loop
| let message; | ||
| for (let [key, value] of Object.entries(failures)) { | ||
| if (key === 'duplicates' && field === 'name') { | ||
| message = 'Detector name is a duplicate'; | ||
| } else if (key === 'missing' && value[0] === field) { | ||
| message = 'This field is required'; | ||
| } else if ((key === 'regex' && field === 'name') || (key === 'format' && field === 'name')) { | ||
| message = 'Valid characters are a-z, A-Z, 0-9, -(hyphen) and _(underscore)'; | ||
| } | ||
| } | ||
| for (let [key, value] of Object.entries(suggestedChanges)) { | ||
| if (key === 'window_delay' && field === 'window_delay') { | ||
| message = 'Window delay should be at least ' + value[0] + ' minutes'; | ||
| } else if (key === 'filter_query' && field === 'filter_query') { | ||
| message = value[0]; | ||
| } else if (key === 'detectionIntervalMax' && field === 'detection_interval') { | ||
| message = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like message could get overwritten here if there was multiple failures and/or suggested changes. How do we propagate all to the user? This may also need to be discussed with UX, where a callout may not be the right solution here if there's several (2+) things the user should be notified of.
| return { | ||
| flyoutProps: { | ||
| 'aria-labelledby': 'createDetectorFlyout', | ||
| maxWidth: 900, |
There was a problem hiding this comment.
Is there a reason for specifying the maxWidth here? Seems that the size prop should be sufficient here
| const detectorFailure = (context) => ({ | ||
| flyoutProps: { | ||
| 'aria-labelledby': 'createDetectorFlyout', | ||
| maxWidth: 700, |
There was a problem hiding this comment.
again, I don't think maxWidth should be necessary here
public/pages/CreateMonitor/containers/DefineMonitor/DefineMonitor.js
Outdated
Show resolved
Hide resolved
| if (aggregationType == 'count') { | ||
| aggregationType = 'value_count'; | ||
| } |
There was a problem hiding this comment.
what is the reason for this? Is there a discrepancy between naming conventions for the count aggregation between alerting and AD, but the rest of the aggregations match up?
| let message = ''; | ||
| for (let [key, value] of Object.entries(validationResponse.failures)) { | ||
| if (key === 'others') { | ||
| message = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can there be multiple failures with a key of others? Also, confused on what others means here, could you explain?
| query: searchQuery, | ||
| }; | ||
| const response = await httpClient.post('../api/alerting/_search', options); | ||
| let maxStamp = _.get(response, 'data.resp.aggregations.max_timefield.value'); |
There was a problem hiding this comment.
What will the maxStamp be if no timestamp is found (empty data)? Will this throw an exception, or just return some default value like 0?
There was a problem hiding this comment.
Currently the visual graph type monitors require a timefield to be created so there will always be one while converting from a visual graph monitor. I could query the index mapping for time field for extra protection and especially when other monitor types are converted.
There was a problem hiding this comment.
Got it, I'm just wondering if no timestamp is found (even if the time field exists) what the query will return. Not sure if ES returns 0 or null on max aggs that are empty. Maybe there will already be issues w/ monitor creation if this is the case.
| <EuiButton | ||
| isLoading={updating} | ||
| onClick={() => this.convertToADConfigs(this.state.monitor)} | ||
| disabled={ | ||
| monitor.ui_metadata.search.searchType !== 'graph' || | ||
| monitor.ui_metadata.search.fieldName === '' || | ||
| monitor.ui_metadata.schedule.timezone | ||
| } | ||
| > | ||
| Create Detector | ||
| </EuiButton> |
There was a problem hiding this comment.
What will the button show if a detector is already created for this monitor? Wondering if it should be changed to a link to the corresponding detector, similar to how AD handles created corresponding monitors.
There was a problem hiding this comment.
At the moment sense the monitor itself isn't directly linked with the newly made detector so the button will still show and users will have the ability to create multiple detectors from the monitor, will discuss this more with UX
There was a problem hiding this comment.
Got it, yeah this may require some backend changes to store some information on a detector created from a monitor.
| import { formikToWhereClause } from '../../../pages/CreateMonitor/containers/CreateMonitor/utils/formikToMonitor'; | ||
| import { MAX_INTERVAl_LENGTH_MINUTES } from '../../../pages/MonitorDetails/containers/utils/helpers'; | ||
|
|
||
| function toString(obj) { |
There was a problem hiding this comment.
This function does multiple work: 1. return period filed of input obj 2.parse input obj to specific time format if it's number type. 3. return '-' for undefined input
Suggest to split this function into two (getPeriod and formatTime, put formatTime as some common utils) and make the function name reveal what exactly it does. Suggest use lodash get function to simplify code.
| super(props); | ||
| } | ||
| render() { | ||
| const featureAttributes = this.props.featureAttributes; |
There was a problem hiding this comment.
Is featureAttributes just one feature, not feature array, right? If yes, how about we make it singular, rename it as featureAttribute or feature
| return ( | ||
| <div> | ||
| <p> | ||
| {' '} |
There was a problem hiding this comment.
removed, originally it helped with styling it but I changed things sense
| <EuiButton | ||
| flush="right" | ||
| disabled={Object.keys(context.failures).length != 0} | ||
| onClick={() => createAndStartDetector(context)} |
There was a problem hiding this comment.
If user click "Create Detector" button , will create detector first, and start to run this detector immediately ?
There was a problem hiding this comment.
Yes, I decided since the main goal behind this feature is to help users adopt AD and ease the set up process, it would be nice to automate the start step after creation from an already existing monitor.
| const detectorId = _id; | ||
| if (ok) { | ||
| try { | ||
| const response = await httpClient.post(`../api/alerting/detectors/${detectorId}/_start`); |
There was a problem hiding this comment.
Should we ask user to confirm should start detector or not after creation ?
There was a problem hiding this comment.
The general idea is to make the creation process as user friendly as possible so giving an option not to start the detector might confuse unexperienced users. Overall this follows the "1-click" design better and especially assuming the data is already being used for a monitor it is likely they would just want to start this. However I see the point in the added confirmation and wait for the actual start, I'll mention it with UX and add to the handoff docs.
| } | ||
| for (let [key, value] of Object.entries(context.suggestedChanges)) { | ||
| if (key === 'detection_interval') { | ||
| let intervalMinutes = Math.ceil(value[0] / 60000) + 1; |
There was a problem hiding this comment.
Why add 1 more minutes as already ceil the value?
There was a problem hiding this comment.
This was done to account for the time that user might take in changing configurations, and confirming everything so I felt as it was safe to add 1 more minute, I could change this however or be be more clear in my reasoning in adding this through the code
There was a problem hiding this comment.
How to calculate the detection interval suggested change ? Detection interval should be some stable value from historical data analysis. Here the suggested value is the minimal interval which contains at least one raw data point ? If yes, we should not add 1 more minute. For example, the index has data for every minute, then the minimal suggested interval should be 1 minute, but with this implementation, we suggest user to use 2 minutes.
Should be reasonable to add 1 more minute for window delay.
| </p> | ||
| <p> | ||
| {' '} | ||
| <b>Aggregation method:</b> {featureAttributes.aggregationType || ''} |
There was a problem hiding this comment.
Why don't we show "Field" and "Aggregation method" as separate columns, like "Feature name" and "State"?
There was a problem hiding this comment.
This was to keep the same style displayed in the configurations page on the anomaly detection plugin since its the same format that the AD plugin follows. I will discuss this further with UX.
| } | ||
| return '-'; | ||
| } | ||
|
|
There was a problem hiding this comment.
General suggestion, this file contains too many things, suggest to refactor it to make the code structure clearer.
tlfeng
force-pushed
the
master
branch
3 times, most recently
from
ce430a5
to
09f93ce
Compare
Description of changes:
These are the front end changes for the AD Everywhere project. The main changes revolve adding the create detector button to the monitor details page which converts the existing monitor configurations to Anomaly detector configurations. The configurations are then validated against the validation API to pre fill the fly out. This button is only useble on visual graph type monitors.
Internal Changes:
External Changes:
Added button:
Example of pre filled flyout:
Example of flyout if query filter is wrong and needs changing:
Example of Detector Failure flyout when there is no way detector can be made due to no historical data:
After Create detector has been clicked on the flyout:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.