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.

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

[Alerting] Adds generic UI for the definition of conditions for Action Groups #83278

Merged
merged 45 commits into from
Nov 20, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Nov 12, 2020

closes #82412

Summary

This PR adds two components to aid in creating a uniform UI for specifying the conditions for Action Groups:

  1. AlertConditions: A component that generates a container which renders custom component for each Action Group which has had its conditions specified.
  2. AlertConditionsGroup: A component that provides a unified container for the Action Group with its name and a button for resetting its condition.

This can be used by any Alert Type to easily create the UI for adding action groups with whichever UI is specific to their component.

For example, in the Always Firing alert we want the user to specify a threshold which dictates which T-Shirt size the Alert Instance is assigned. When the randomly generated number exceeds the threshold, the Alert Instance is assigned to that action group:

Screenshot 2020-11-12 at 16 34 55

This is achieved in the Always Firing alert type by utilising these components:

<AlertConditions
headline={'Set different thresholds for randomly generated T-Shirt sizes'}
actionGroups={actionGroups.map((actionGroup) =>
Number.isInteger(thresholds[actionGroup.id as AlwaysFiringActionGroupIds])
? {
...actionGroup,
conditions: thresholds[actionGroup.id as AlwaysFiringActionGroupIds]!,
}
: actionGroup
)}
onInitializeConditionsFor={(actionGroup) => {
setAlertParams('thresholds', {
...thresholds,
...pick(DEFAULT_THRESHOLDS, actionGroup.id),
});
}}
>
<AlertConditionsGroup
onResetConditionsFor={(actionGroup) => {
setAlertParams('thresholds', omit(thresholds, actionGroup.id));
}}
>
<TShirtSelector
setTShirtThreshold={(actionGroup) => {
setAlertParams('thresholds', {
...thresholds,
[actionGroup.id]: actionGroup.conditions,
});
}}
/>
</AlertConditionsGroup>
</AlertConditions>

This approach ensures the different Alert Types can easily create a uniform UX which is shared throughout the framework, but also leaved enough flexibility to allow Alert Types to provide their own custom labeling and components for specifying the conditions (as required for #83270).

Checklist

Delete any items that are not applicable to this PR.

For maintainers

gmmorris and others added 24 commits November 9, 2020 13:45
* master: (39 commits)
  Fix ilm navigation (elastic#81664)
  [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927)
  [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509)
  Index patterns api - load field list on server (elastic#82629)
  New events resolver (elastic#82170)
  [App Search] Misc naming tech debt (elastic#82770)
  load empty_kibana in test to have clean starting point (elastic#82772)
  Remove data <--> expressions circular dependencies. (elastic#82685)
  Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905)
  Add alerting as codeowners to related documentation folder (elastic#82777)
  Add captions to user and space grid pages (elastic#82713)
  add alternate path for x-pack/Cloud test for Lens (elastic#82634)
  Uses asCurrentUser in getClusterUuid (elastic#82908)
  [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519)
  Fix test import objects (elastic#82767)
  [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662)
  Update alert type selection layout to rows instead of grid (elastic#73665)
  Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817)
  Update grunt and related packages (elastic#79327)
  Allow the repository to search across all namespaces (elastic#82863)
  ...
…na into alerts/stack-alerts-public

* 'alerts/stack-alerts-public' of github.com:gmmorris/kibana:
  [Security Solution] Fix DNS Network table query (elastic#82778)
  [Workplace Search] Consolidate groups routes (elastic#83015)
  Adds cloud links to user menu (elastic#82803)
  [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023)
  [App Search] Added the log retention panel to the Settings page (elastic#82982)
  [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006)
  [DOCS] Consolidates drilldown pages (elastic#82081)
  [Maps] add on-prem EMS config (elastic#82525)
  migrate i18n mixin to KP (elastic#81799)
  [bundle optimization] fix imports of react-use lib (elastic#82847)
  [Discover] Add metric on adding filter (elastic#82961)
  [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829)
  skip flaky suite (elastic#82804)
  Fix SO query for searching across spaces (elastic#83025)
  renaming built-in alerts to Stack Alerts (elastic#82873)
  [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278)
  [Visualizations] Remove kui usage (elastic#82810)
  [Visualizations] Make the icon buttons labels more descriptive (elastic#82585)
  [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694)
:
…ts-public

* upstream/master: (57 commits)
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  [ts/checkTsProjects] produce a more useful error message (elastic#83209)
  [kbnClient] retry updating config if necessary (elastic#83205)
  I accidentally removed this line in a recent PR (elastic#83201)
  Don't make the caller do work the function can do (elastic#83180)
  [App Search] Update EngineRouter & EngineNav to use EngineLogic (elastic#83138)
  [Workplace Search] Add routes for Sources (elastic#83125)
  Update logstash pipeline management to use system index APIs (elastic#80405)
  [ML] Replace EuiBasicTable with EuiInMemoryTable (elastic#83057)
  [Metrics UI] Add basic interaction and shell for node details overlay (elastic#82013)
  [App Search] Added the log retention confirmation modal to the Settings page (elastic#83009)
  [docs] Fix create map title in import geospatial page (elastic#83172)
  ...
* master:
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
… alerts/action-groups-as-conditions

* origin/alerts/stack-alerts-public: (91 commits)
  removed import from plugin code as it causes FTR to fail
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
  make defaulted field non maybe
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  ...
* master:
  [Ingest Manager] Lift up registry/{stream,extract} functions (elastic#83239)
  [Reporting] Move "common" types and constants to allow cross-plugin integration (elastic#83198)
  [Lens] Add suffix formatter (elastic#82852)
  [App Search] Version documentation links (elastic#83245)
  Use saved object references for dashboard drilldowns (elastic#82602)
  Btsymbala/registered av (elastic#81910)
  [APM] Errors table for service overview (elastic#83065)
* master:
  [ML] Update apidoc config with the Trained models endpoints  (elastic#83274)
  [Fleet] IngestManager Plugin interface for registering UI extensions (elastic#82783)
  [Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin (elastic#82951)
Comment on lines 21 to 30
alertFlyoutVisible && alertTypeId ? (
<AlertAdd
addFlyoutVisible={alertFlyoutVisible}
consumer="uptime"
setAddFlyoutVisibility={setAlertFlyoutVisibility}
alertTypeId={alertTypeId}
// if we don't have an alert type pre-specified, we need to
// let the user choose
canChangeTrigger={!alertTypeId}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like Flyout will not open if alertTypeId isn't selected? but i guess that use case will never happen inside uptime, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, exactly - we need to fix some buggy race conditions on our end which allow Uptime to accidentally render half a flyout that thinks no type is selected (because alertTypeId is undefined) and half that thinks you have selected one.

Uptime is the only place (that I'm aware of) where this behaviour is relied on... so the idea is to patch it this way until we can address it properly .

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Nov 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Functionality wise it looks good, my only concern is that there is white content screen when flyout opens, first time it's pretty long, on subsequent openings, it still pretty noticeable, this is new behaviour, either we can improve this or we should at-least show the loading.

Though i am not sure, if this behaviour needs to be fixed in this PR, i leave it up to you.

image

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.

Looks awesome!

* master: (51 commits)
  [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439)
  adds xpack.security.authc.selector.enabled setting (elastic#83551)
  skip flaky suite (elastic#77279)
  [ML] Improve support for script and aggregation fields in anomaly detection jobs (elastic#81923)
  [Workplace Search] Migrate SourcesLogic from ent-search (elastic#83544)
  [ML] Add UI test for feature importance features (elastic#82677)
  [Maps] Improve icons for all layer types (elastic#83503)
  Replace experimental badge with Beta (elastic#83468)
  [Fleet][EPM] Unified install and archive (elastic#83384)
  Move src/legacy/server/keystore to src/cli (elastic#83483)
  Used SO for saving the API key IDs that should be deleted (elastic#82211)
  [Uptime] Mock implementation to account for math flakiness test (elastic#83535)
  [Workplace Search] Enable check for org context based on URL (elastic#83487)
  [App Search] Added all Document related routes and logic (elastic#83324)
  [Alerting UI] Fix console error when setting connector params (elastic#83333)
  [Discover] Allow custom name for fields via index pattern field management (elastic#70039)
  [Uptime] Fix monitor list down histogram (elastic#83411)
  remove headers timeout hack, rely on nodejs timeouts (elastic#83419)
  [ML] Update console autocomplete for ML data frame evaluate API (elastic#83151)
  [Lens] Color in dimension trigger (elastic#76871)
  ...
@ymao1
Copy link
Contributor

ymao1 commented Nov 18, 2020

Typing changes look good! My only nit is the naming of the InitialAlert type since initialAlert is a variable we use in other places (like the AlertEdit component, where the type of initialAlert is actually Alert) but I get the intent so it's not a big deal.

@gmmorris
Copy link
Contributor Author

Typing changes look good! My only nit is the naming of the InitialAlert type since initialAlert is a variable we use in other places (like the AlertEdit component, where the type of initialAlert is actually Alert) but I get the intent so it's not a big deal.

Yeah, I think we should rename the variable, rather than the type, in this case.
I was thinking that could change when we refactor these components in #83417 as it will require changes in the plugins using the flyouts. I hope that refactoring will break these components down to smaller components that don't rely on state as much and we might be able to correctly rename that to something more "correct".

* master: (60 commits)
  Forward any registry cache-control header for files (elastic#83680)
  Revert "[Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578)"
  [Security Solution][Detections] Fix adding an action to detection rules (elastic#83722)
  Make expectSnapshot available in all functional test runs (elastic#82932)
  Skip failing cypress test
  Increase bulk request timeout during esArchiver load (elastic#83657)
  [data.search] Server-side background session service (elastic#81099)
  [maps] convert VectorStyleEditor to TS (elastic#83582)
  Revert "[App Search] Engine overview layout stub (elastic#83504)"
  Adding documentation for global action configuration options (elastic#83557)
  [Metrics UI] Optimizations for Snapshot and Inventory Metadata (elastic#83596)
  chore(NA): update lmdb store to v0.8.15 (elastic#83726)
  [App Search] Engine overview layout stub (elastic#83504)
  [Workplace Search] Update SourceIcon to match latest changes in ent-search (elastic#83714)
  [Enterprise Search] Rename React Router helpers (elastic#83718)
  [Maps] Add 'crossed' & 'exited' events to tracking alert (elastic#82463)
  Updating code-owners to use new core/app-services team names (elastic#83731)
  Add Managed label to data streams and a view switch for the table (elastic#83049)
  [Maps] Add query bar inputs to geo threshold alerts tracked points & boundaries (elastic#80871)
  fix(NA): search examples kibana version declaration (elastic#83182)
  ...
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

})
);
return newAlert;
if (isValidAlert(alert, errors)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the addition of isValidAlert check here. Does it mean there's a scenario the save code can get executed while the alert hasErrors (disabled save button) or just for safety?

Copy link
Contributor Author

@gmmorris gmmorris Nov 19, 2020

Choose a reason for hiding this comment

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

Type wise, yes, but not UX wise.
I added the check to safely infer that this is in fact an Alert and not an InitialAlert.
This is what happens when we fiddle with types and state the way we have... If we break these components down we might be able to improve that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks 👍

* master:
  skip "Dashboards linked by a drilldown are both copied to a space" (elastic#83824)
  [alerts] adds action group and date to mustache template variables for actions (elastic#83195)
  skip flaky suite (elastic#79389)
  [DOCS] Reallocates limitations to point-of-use (elastic#79582)
  [Enterprise Search] Engine overview layout stub (elastic#83756)
  Disable exporting/importing of templates.  Optimize pitch images a bit (elastic#83098)
  [DOCS] Consolidates plugins (elastic#83712)
  [ML] Space management UI (elastic#83320)
  test just part of the message to avoid updates (elastic#83703)
  [Data Table] Remove extra column in split mode (elastic#83193)
  Improve snapshot error messages (elastic#83785)
  skip flaky suite (elastic#83773)
  skip flaky suite (elastic#83771)
  skip flaky suite (elastic#65278)
  skip flaky suite (elastic#83793)
  [Task Manager] Ensures retries are inferred from the schedule of recurring tasks (elastic#83682)
  [index patterns] improve index pattern cache (elastic#83368)
  [Fleet] Rename ingestManager plugin ID fleet (elastic#83200)
  fixed pagination in connectors list (elastic#83638)
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 275 277 +2

Async chunks

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

id before after diff
triggersActionsUi 1.4MB 1.4MB +941.0B

Page load bundle

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

id before after diff
triggersActionsUi 143.1KB 152.2KB +9.1KB

History

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

@gmmorris gmmorris merged commit 8aa7e13 into elastic:master Nov 20, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 20, 2020
…n Groups (elastic#83278)

This PR adds two components to aid in creating a uniform UI for specifying the conditions for Action Groups:
1. `AlertConditions`: A component that generates a container which renders custom component for each Action Group which has had its _conditions_ specified.
2. `AlertConditionsGroup`: A component that provides a unified container for the Action Group with its name and a button for resetting its condition.

This can be used by any Alert Type to easily create the UI for adding action groups with whichever UI is specific to their component.
gmmorris added a commit that referenced this pull request Nov 20, 2020
…n Groups (#83278) (#83896)

This PR adds two components to aid in creating a uniform UI for specifying the conditions for Action Groups:
1. `AlertConditions`: A component that generates a container which renders custom component for each Action Group which has had its _conditions_ specified.
2. `AlertConditionsGroup`: A component that provides a unified container for the Action Group with its name and a button for resetting its condition.

This can be used by any Alert Type to easily create the UI for adding action groups with whichever UI is specific to their component.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 20, 2020
* master: (38 commits)
  [ML] Data frame analytics: Adds functionality to map view (elastic#83710)
  Add usage collection for savedObject tagging (elastic#83160)
  [SECURITY_SOLUTION] 145: Advanced Policy Tests (elastic#82898)
  [APM] Service overview transactions table (elastic#83429)
  [ML] Fix Single Metric Viewer not loading if job is metric with no partition (elastic#83880)
  do not export types from 3rd party modules as 'type' (elastic#83803)
  [Fleet] Allow to send SETTINGS action (elastic#83707)
  Fixes Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details·ts - Actions and Triggers app Alert Details Alert Instances renders the active alert instances (elastic#83478)
  [Uptime]Reduce chart height on monitor detail page (elastic#83777)
  [APM] Prefer `APIReturnType` over `PromiseReturnType` (elastic#83843)
  [Observability] Fix telemetry for Observability Overview (elastic#83847)
  [Alerting] Adds generic UI for the definition of conditions for Action Groups (elastic#83278)
  ensure workload agg doesnt run until next interval when it fails (elastic#83632)
  [ILM] Policy form should not throw away data (elastic#83077)
  [Monitoring] Stop collecting Kibana Usage in bulkUploader (elastic#83546)
  [TSVB] fix wrong imports (elastic#83798)
  [APM] Correlations UI POC (elastic#82256)
  list all the refs in  tsconfig.json (elastic#83678)
  Bump jest (and related packages) to v26.6.3 (elastic#83724)
  Functional tests - stabilize reporting tests for cloud execution (elastic#83787)
  ...
@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Dec 16, 2020
@gmmorris
Copy link
Contributor Author

Decided to remove the needs_docs label on this as it is documented in the Developer Docs and as an end user can't use these components directly (they are used by Alert Type implementors).
It doesn't make sense to add them to user docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There's no framework level UI for defining conditions for Action Groups
6 participants