Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Discover] Validate if Data View time field exists on Alert creation / editing #146324

Merged
merged 9 commits into from Dec 14, 2022
Merged

[Discover] Validate if Data View time field exists on Alert creation / editing #146324

merged 9 commits into from Dec 14, 2022

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Nov 24, 2022 •
edited

Summary

Closes #135806

This PR adds optional timeField param for Discover alert and adding validation data view if it time based.

AD61D10F-6278-429C-B69D-C1952BB0A3C1_4_5005_c

How to test

  • Open Alerts in Discover
  • Select non time based data view
  • Try to save the rule. You should see error message.

Checklist

@dimaanj dimaanj added Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.7.0 labels Nov 24, 2022
@dimaanj dimaanj self-assigned this Nov 24, 2022
@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 1, 2022

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 2, 2022

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 5, 2022

@elasticmachine merge upstream

@dimaanj dimaanj marked this pull request as ready for review December 5, 2022 13:35
@dimaanj dimaanj requested review from a team as code owners December 5, 2022 13:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@ymao1
Copy link
Contributor

ymao1 commented Dec 5, 2022

@dimaanj Wondering if it's possible to just filter out data views with no time field from the suggestions list? Since the rule won't work with them, why show them at all? Or provide some sort of indicator on the list whether a data view has a time field. I'm wondering if this might cause frustration when users click to select data views and keep getting errors about not having a time field?

@kertal
Copy link
Member

kertal commented Dec 5, 2022

@dimaanj Wondering if it's possible to just filter out data views with no time field from the suggestions list? Since the rule won't work with them, why show them at all? Or provide some sort of indicator on the list whether a data view has a time field. I'm wondering if this might cause frustration when users click to select data views and keep getting errors about not having a time field?

this would be great, but I think it's much more effort (right @dimaanj ) but would be good to create an issue for a potential follow up

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 6, 2022

this would be great, but I think it's much more effort (right @dimaanj ) but would be good to create an issue for a potential follow up

Created #147088

ymao1
ymao1 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified that I am unable to create new KQL rules using data views w/o the time field and I also get an error when editing existing KQL rules that already use data views w/o the time field.

x-pack/plugins/stack_alerts/public/rule_types/es_query/validation.ts Show resolved Hide resolved
x-pack/plugins/stack_alerts/server/rule_types/es_query/lib/fetch_search_source_query.ts Outdated Show resolved Hide resolved
dej611
dej611 approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review only for @elastic/kibana-visualizations owned code

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 14, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackAlerts 74.7KB 74.9KB +246.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
stackAlerts 14.0KB 14.2KB +215.0B
unifiedSearch 50.5KB 50.6KB +51.0B
total +266.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 60 66 +6
osquery 109 115 +6
securitySolution 445 451 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 69 75 +6
osquery 110 117 +7
securitySolution 521 527 +6
total +21

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dimaanj

@dimaanj dimaanj merged commit 6b5ab8f into elastic:main Dec 14, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 14, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 14, 2022
@jloleysens
Merge branch 'main' into only-migrate-if-needed-KBNA-4488
60d2987 
* main: (21 commits)
  [Profiling] Remove link to 'Other' bucket (elastic#147523)
  [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265)
  [DOCS] Updates what's new pages (elastic#147483)
  [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361)
  [Guided onboarding] Update guide IDs (elastic#147348)
  [Synthetics] Add synthetics settings alerting default (elastic#147339)
  [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212)
  [Cases] Save draft user comment (elastic#146327)
  [API Docs] Fix `--plugin` filter (elastic#147500)
  [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439)
  Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503)
  [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324)
  [Discover] Fix Discover navigation from Lens embeddable (elastic#147000)
  Allow users to Update API Keys (elastic#146237)
  Update dependency xstate to ^4.35.0 (main) (elastic#147463)
  [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429)
  [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432)
  [APM] Switching service groups from grid to flex layout (elastic#147448)
  [Fleet] Add missing endpoints to openApi specs (elastic#147452)
  [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253)
  ...
nreese pushed a commit to nreese/kibana that referenced this pull request Dec 16, 2022
@dimaanj @kibanamachine @nreese
[Discover] Validate if Data View time field exists on Alert creation …
f718fec 
…/ editing (elastic#146324)

## Summary

Closes elastic#135806

This PR adds optional `timeField` param for Discover alert and adding
validation data view if it time based.


![AD61D10F-6278-429C-B69D-C1952BB0A3C1_4_5005_c](https://user-images.githubusercontent.com/39378793/205312590-0392cd2e-740e-4e3e-ba17-712e0696eef3.jpeg)

### How to test
- Open `Alerts` in Discover
- Select non time based data view
- Try to save the rule. You should see error message.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 approved these changes

@ymao1 ymao1 ymao1 approved these changes

Assignees

@dimaanj dimaanj

Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.7.0
Projects
None yet
Milestone
No milestone
7 participants
@dimaanj @elasticmachine @ymao1 @kertal @kibana-ci @dej611 @kibanamachine