-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add geo containment tracking alert type #84151
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
@thomasneirynck Might also be worth changing our experimental flag in this PR to cover both alert types and make more generic. Thoughts? Update: Discussed offline and agreed to update config value to cover both. |
…> enableGeoAlerts
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
🎉 I think this has a great scope.
imho, the containment-approach fits in really nicely with the overall alerting framework:
- this monitors the state of a vehicle over time. the state is its location relative to a given shape. Compare this to e.g. monitoring the health of a system.
- a vehicle can be in a given state across multiple time-windows. Compare this e.g. to how a server may be in "danger-state" for multiple windows
- containment "resolves" when the vehicle no longer is in a given-state, ie. the vehicle is "outside".
- the UX forms of the containment-alert-type does not clash with the surround forms of the alerting-frameword.. In contrast, the UX-form of the geo-threshold alert-type do significantly clash with the surrounding forms. The latter has a lot of options which do not make sense (e.g. what does it mean to throttle messaging for thresholds? what does it mean to notify on resolved-action-group?)
With containment, the alerting framework then handles nuances in the messaging:
- Alerts resolving allows us to only message on "exiting"-events. This is handled by the ability to tie an action to a "resolved" action-group only.
- Ability to throttle messages allows us only to notify when a vehicle enters a shape (pending Ability to throttle alert instances until action group changes #50077)
- In the future, we could even remove the tuple-approach which explicitly ties an alert to a shape-vehicle combo, and instead tie an alert to a vehicle only (pending [Alerting] Add a possibility to define a custom status for the alert instances #78981)
Outstanding known issues:
On geo-containment alert side:
- dealing with laggy data: (ie. ingest is laggy, causing the vehicle-time to precede the index time quite a bit. in this scenario, the alert-window executes before the data for that window is actually indexed).
- dealing with sparse data: vehicles do not ping on a regular frequency, causing their "current"-location (in the real world) to be equivalent to the last-known location (by Elasticsearch). In such a case, the moving interval window is flagging these vehicles as resolved, even though they are still inside.
Overall performance considerations:
- under heavy load (thousands of alerts per window), the alerting-window frequency slows down significantly. Will be discussed with alerting-team on how to proceed: cc @gmmorris @YulNaumenko
@@ -16,8 +17,9 @@ export function registerAlertTypes({ | |||
alertTypeRegistry: TriggersAndActionsUIPublicPluginSetup['alertTypeRegistry']; | |||
config: Config; | |||
}) { | |||
if (config.enableGeoTrackingThresholdAlert) { | |||
if (config.enableGeoAlerts) { |
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.
+1 on grouping them together
containingBoundaryId: shapeLocationId, | ||
containingBoundaryName, | ||
}; | ||
const alertInstanceId = `${entityName}-${containingBoundaryName}`; |
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.
nice. very intuitive now in containment-alert
], | ||
}; | ||
|
||
export const ParamsSchema = schema.object({ |
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.
+1 I think this probsbly capture what we would like in a message. cc @kmartastic Anything else?
aggs: { | ||
shapes: { | ||
filters: { | ||
other_bucket_key: OTHER_CATEGORY, |
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.
we could remove having this other-bucket altogether, because we are explicitly filtering out these results later on:) . This may be hypothetically useful for more advanced fucntionalities later down the line (e.g. tracking vehicle-location through state)
idk, up2you if you would like to remove. It does negatively impact the query, creating buckets when they are not needed. in terms of code, footprint is minor.
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.
Yep, I considered this too. I only left it in because it was going to be the only query mod required for this PR and I happen to know I want this information for the forthcoming PR that adds better handling for if/when an alert instance goes into a resolved state. Basically it gives a firm "We've received an update and confirmed this is not in an area of interest" message. Agreed this is a performance hit in the meantime and should be removed if not used but I'm fairly confident it will be. Good to document this decision here though 👍
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.
Code LGTM. Tested locally and it works great!
* master: (28 commits) [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891) Only attempt to rollover signals index if version < builtin version (elastic#84982) skip flaky suite (elastic#84978) skip lens rollup tests Add geo containment tracking alert type (elastic#84151) Changed rollup tests to use test user rather than elastic super user. (elastic#79567) skip 'should allow creation of lens xy chart' elastic#84957 [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810) [Maps] geo line source (elastic#76572) [data.search] Move search method inside session service and add tests (elastic#84715) skip lens drag and drop test. elastic#84941 [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554) [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640) [Lens] Fix error when selecting the current field again (elastic#84817) [Metrics UI] Add metadata tab to node details flyout (elastic#84454) [CI] Enables APM collection (elastic#81731) [Workplace Search] Migrate Sources Schema tree (elastic#84847) Disable checking for conflicts when copying saved objects (elastic#83575) [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368) y18n 4.0.0 -> 4.0.1 (elastic#84905) ...
Introduce new alert type
Tracking containment
to alerts. Differentiated fromTracking threshold
by the following features:Generally, these instructions will apply for testing this PR with a couple of exceptions:
For 6, use the following mappings:
For 9, use this indexing structure: