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

Add support for geo_shape fields as the entity geospatial field when creating tracking containment alerts #164100

Merged
merged 27 commits into from Aug 23, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 16, 2023

Closes #163996

To test

  1. Checkout fake tracks geo_shape branch
  2. run npm install
  3. run node ./generate_tracks.js
  4. in kibana, create tracks* data view
  5. create map, use "create index" and draw boundaries that intersect tracks. See screen shot
    Screen Shot 2023-08-17 at 2 49 52 PM
  6. create geo containment alert where entity index is tracks* and boundaries index is boundaries.
  7. Verify alerts get generated with entity geo_shape locations

@nreese
Copy link
Contributor Author

nreese commented Aug 17, 2023

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review August 17, 2023 22:37
@nreese nreese requested review from a team as code owners August 17, 2023 22:37
@nreese nreese added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Feature:Maps v8.11.0 labels Aug 17, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese
Copy link
Contributor Author

nreese commented Aug 21, 2023

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Aug 21, 2023

@elasticmachine merge upstream

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! just one thing I'm unsure about.

code review and tested with geo_shape faketracks.

@nreese
Copy link
Contributor Author

nreese commented Aug 21, 2023

@elasticmachine merge upstream

@pmuellr
Copy link
Member

pmuellr commented Aug 22, 2023

Poking around at ZDT/migration issues:

  • at first I thought context variable entityLocation was being changed to string | number[], but it appears that's just the internal implementation. Seems like it will always be a string, and the support for existing shapes is the same string. So seems fine. In the future, there will be other "shapes" beyond POINT, like ... SHAPE or something ...
  • There is a shape change in x-pack/plugins/stack_alerts/server/rule_types/geo_containment/types.ts, with location widening from number[] to number[] | string. Seems like this is the instance state, persisted In the rule's task document. So in theory, during a ZDT upgrade could have an old Kibana read this, and not expect the string. So feels like we need to model/version this, or whatever. I think this is the first of these we're dealing with!

@nreese
Copy link
Contributor Author

nreese commented Aug 22, 2023

@pmuellr could you provide some more details around the comment below. What actions need to happen to resolve this?

So feels like we need to model/version this, or whatever. I think this is the first of these we're dealing with!

I could add a new field to store string version. That would keep location the same. Would that be an acceptable solution?

@nreese
Copy link
Contributor Author

nreese commented Aug 22, 2023

@elasticmachine merge upstream

@pmuellr
Copy link
Member

pmuellr commented Aug 22, 2023

I could add a new field to store string version. That would keep location the same. Would that be an acceptable solution?

That seems like the best solution for now. We're working through how to version this stuff, so that in the future we would know to not run a "upgraded" rule on an "old" kibana, so something like what's in the PR could be made to work, with some version labeling.

The other thought is - how well does old code cope with a string? Worse case is the throws an error, and we may need to do some Dev Console "surgery" to fix it up (probably delete the task document). Maybe that's tolerable?

@nreese
Copy link
Contributor Author

nreese commented Aug 22, 2023

ZDT addressed with 4ecf124.

new state stored in locationWkt field. Alert executor continues to populate legacy location field with valid data. With these changes, non-updated kibana nodes can handle alert start from updated kibana nodes.

@nreese
Copy link
Contributor Author

nreese commented Aug 23, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #28 / console app console autocomplete feature anti-regression watchdogs should activate auto-complete for a single character immediately following a slash in URL

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
securitySolution 15.7MB 15.7MB +6.4KB
securitySolutionServerless 249.3KB 255.5KB +6.2KB
stackAlerts 77.2KB 77.2KB +13.0B
total +12.7KB

Page load bundle

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

id before after diff
securitySolutionServerless 38.0KB 38.0KB +2.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
stackAlerts 26 27 +1

Total ESLint disabled count

id before after diff
stackAlerts 26 27 +1

History

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

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@nreese nreese merged commit 3393d87 into elastic:main Aug 23, 2023
24 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Maps release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for geo_shape fields as the entity geospatial field when creating tracking containment alerts
6 participants