-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
Do we want to get UX input on any of the changes and wording? |
Ping UX and tech writer. Waiting for reply. |
Add another callout based on UX designer's suggestion: "consider diaplaying something within the Add filter section as the user may scroll past the callout’s visibility." Change callout message as tech writer's 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.
LGTM, thanks for adding this feature.
onCreateOption={(createdOption: string) => { | ||
const normalizedOptions = createdOption.trim(); | ||
if (!normalizedOptions) return; | ||
form.setFieldValue('timeField', normalizedOptions); | ||
}} |
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.
why allow user to create option? what if user inputs invalid timestamp field?
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.
Similar questions: what if user input invalid filed in custom expression of filter and feature definition.
If user input invalid filed, like they input a filed which is string not timestamp, or the filed not exists, the detector just can't get any data to run.
A better way is we can pop up some warning message and block create detector with invalid parameters.
Amit is working on auto AD project. We can discuss that design, a common sanity checker component is useful. And I think that can be a part of Amit's project. Created a backlog issue to track this. opendistro-for-elasticsearch/anomaly-detection#179
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.
why allow user to create option?
This change is not a final solution, just some workaround to support remote cluster indices.
For local indices, user still can select filed from dropdown list. For remote cluster indices, ES doesn't support listing remote cluster's indices, not get index mapping. We can't get remote cluster index's fields. So for remote cluster indices, user need to input filed manually.
Once ES supported listing remote indices and get remote index's mapping, we will query remote index mapping and user can choose filed like local indices.
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.
Thanks. Btw, why timestamp field from remote index is not visible?
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.
Thanks. Btw, why timestamp field from remote index is not visible?
For local index, we can get its index mapping and parse which filed is timestamp type. But for remote cluster index, we can't get its index mapping. So we don't know which filed is timestamp.
Issue #, if available:
Description of changes:
Community has requirements to create detector on top of remote cluster’s indices, refer to Github issue (#215). User can do this with create detector public API, but can’t choose/input remote cluster indices and its fields on Kibana.
Currently Elasticsearch doesn’t support listing indices on remote cluster, nor get index mapping. So we just support remote cluster indices by allowing user input index and filed name manually.
Create/edit detector page
Edit feature page
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.