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

[Maps] Fix threshold alert issue resolving nested fields #83577

Merged
merged 5 commits into from
Nov 18, 2020
Merged

[Maps] Fix threshold alert issue resolving nested fields #83577

merged 5 commits into from
Nov 18, 2020

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Nov 17, 2020

Geo threshold alerts incorrectly handle nested date and geo fields. Since ES queries use dot notation for single keys, the following lodash usage works incorrectly for something like geo.coordinates:

❌ entityHits.hits.hits[0].fields.${geoField}[0] -> entityHits.hits.hits[0].fields.geo.coordinates[0]

Instead, to treat the entire dot-separated field as one field, as is the case in the response, the following should be used:

✔️ entityHits.hits.hits[0].fields["${geoField}"][0] -> entityHits.hits.hits[0].fields["geo.coordinates"][0]

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:Alerting labels Nov 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

thomasneirynck
thomasneirynck approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx for quick fix.

question wrt. test, maybe I'm missing something there though :)

x-pack/plugins/stack_alerts/server/alert_types/geo_threshold/geo_threshold.ts
@@ -38,7 +38,7 @@ export function transformResults(
return _.map(subBuckets, (subBucket) => {
const locationFieldResult = _.get(
subBucket,
`entityHits.hits.hits[0].fields.${geoField}[0]`,
`entityHits.hits.hits[0].fields["${geoField}"][0]`,
Copy link
Contributor

Choose a reason for hiding this comment

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

clever hack. I had no idea _.get could work with this syntax.

...ins/stack_alerts/server/alert_types/geo_threshold/tests/es_sample_response_with_nesting.json Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor

nreese commented Nov 17, 2020

Does your fix work if the field is an object and if the field has a dot in the name? We had to handle similar problems when pulling fields out of hits for documents source. Below are the test cases we ensured worked.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/maps/common/elasticsearch_util/elasticsearch_geo_utils.test.js#L167

@kindsun
Copy link
Contributor Author

kindsun commented Nov 17, 2020

Does your fix work if the field is an object and if the field has a dot in the name? We had to handle similar problems when pulling fields out of hits for documents source. Below are the test cases we ensured worked.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/maps/common/elasticsearch_util/elasticsearch_geo_utils.test.js#L167

Thanks for the heads up! Did some investigation, it works with both fields using a dot, like geo.coordinates or nested like { geo: { coordinates: ...}}. In this case we're grabbing data from the returned values assigned to docvalue_fields. These are specified upfront in the initial query and match both the dot and nested forms on the ES side. By the time we see it for this function, it's always just what we specified: geo.coordinates.

@kindsun
Copy link
Contributor Author

kindsun commented Nov 17, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 42848 42849 +1

History

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

@kindsun kindsun merged commit 982639f into elastic:master Nov 18, 2020
@kindsun kindsun deleted the fix-threshold-alert-issue-resolving-nested-fields branch November 18, 2020 00:48
This was referenced Nov 18, 2020
Merged
Merged
kindsun pushed a commit to kindsun/kibana that referenced this pull request Nov 18, 2020
@kibanamachine
[Maps] Fix threshold alert issue resolving nested fields (elastic#83577)
910155a 
Co-authored-by: Kibana Machine <[email protected]>
kindsun pushed a commit that referenced this pull request Nov 18, 2020
@kibanamachine
[7.x] [Maps] Fix threshold alert issue resolving nested fields (#83577)…
ac31e63 
… (#83605)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
kindsun pushed a commit that referenced this pull request Nov 18, 2020
@kibanamachine
[7.10] [Maps] Fix threshold alert issue resolving nested fields (#83577…
7a0330c 
…) (#83606)

Co-authored-by: Kibana Machine <[email protected]>
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 18, 2020
@phillipb
Merge branch 'master' of github.com:elastic/kibana into add-metrics-t…
2def566 
…o-node-details

* 'master' of github.com:elastic/kibana: (65 commits)
  update chromedriver dependency to 87 (elastic#83624)
  [TSVB] use new Search API for rollup search (elastic#83275)
  [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438)
  [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302)
  Add tag bulk action context menu (elastic#82816)
  [code coverage] adding plugin to flush coverage data (elastic#83447)
  [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413)
  Added eventBus to trigger and listen plotHandler event (elastic#83435)
  [Runtime fields] Editor phase 1 (elastic#81472)
  [Maps] Fix threshold alert issue resolving nested fields (elastic#83577)
  chore(NA): remove usage of unverified es snapshots (elastic#83589)
  [DOCS] Adds Elastic Contributor Program link (elastic#83561)
  Upgrade EUI to v30.2.0 (elastic#82730)
  Don't show loading screen during auto-reload (elastic#83376)
  Functional tests - fix esArchive mappings with runtime fields (elastic#83530)
  [deb/rpm] Create keystore after installation (elastic#76465)
  [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144)
  [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455)
  [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439)
  adds xpack.security.authc.selector.enabled setting (elastic#83551)
  ...
@kindsun kindsun mentioned this pull request Nov 18, 2020
Closed
20 tasks
@bhavyarm
Copy link
Contributor

bhavyarm commented Dec 3, 2020

@aaronjcaldwell how do I test this please? Thanks!

@kindsun
Copy link
Contributor Author

kindsun commented Dec 3, 2020

Hello @bhavyarm! Spinning up a Geo Threshold Alert environment (required to test this fix) is currently a very involved task requiring several steps outlined here. Since this functionality is still experimental, I'm not sure if it's critical for you to test or not. Happy to walk you through the steps offline if needed. We'd likely need about an hour.

@bhavyarm
Copy link
Contributor

bhavyarm commented Dec 3, 2020

@aaronjcaldwell oh wow ok. It looks super interesting. will talk to Lee and buzz you. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck approved these changes

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:Alerting release_note:fix v7.10.1 v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kindsun @elasticmachine @nreese @kibanamachine @bhavyarm @thomasneirynck