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

[Maps] Fix geo alerts handling of multi-fields #100348

Merged
merged 8 commits into from
Jun 14, 2021

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented May 19, 2021

Fixes #100330. Prior to this fix, index pattern fields were screened only for field format. This PR adds a check for aggregatability and ignores fields that can't be aggregated in multi-fields and otherwise

Future work could explore support for multi-fields but for now it makes more sense to prevent users from leveraging unsupported capabilities.

Before fix:

Screenshot_20210519_094216

After fix:

Screenshot_20210519_101705

@kindsun kindsun marked this pull request as ready for review May 19, 2021 19:55
@kindsun kindsun requested a review from a team as a code owner May 19, 2021 19:55
@elasticmachine
Copy link
Contributor

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

@kindsun
Copy link
Contributor Author

kindsun commented May 19, 2021

Tested locally using a modified version of https://github.com/thomasneirynck/faketracks containing a multi-field for the entity id. Here were the mappings used for this field:

"entity_id": {
    "type": "text",
    "fields": {
        "raw": {
            "type":  "keyword"
        }
    }
},

The primary issue wasn't actually a field being a multi-field, but instead was aggragateability. For instance, in the case above, both entity_id and entity_id.raw are fields. entity_id.raw is aggragateable and a multi-field and works as expected, whereas entity_id, which is neither, doesn't.

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.

Can you add snapshot test for this component ? Should probably show the filtering in action (I think).

tested, filters out the non-aggregatable text-fields.

e.g.:

     "ephemeral_id" : {
              "type" : "text",
              "fields" : {
                "keyword" : {
                  "type" : "keyword",
                  "ignore_above" : 256
                }
              }
            },

image

@thomasneirynck
Copy link
Contributor

remove the 7.12.2 label since we won't have a 7.12.2 release (at least. as it stands now).

I'd try to add test-coverage, given we are patching a release.

@thomasneirynck
Copy link
Contributor

this will not make 7.13.1, moving to 7.13.2

@thomasneirynck thomasneirynck changed the title Fix geo alerts handling of multi-fields [Maps] Fix geo alerts handling of multi-fields May 28, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 662.9KB 663.1KB +187.0B
Unknown metric groups

References to deprecated APIs

id before after diff
stackAlerts 101 104 +3

History

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

@kindsun kindsun merged commit 6e0aed7 into elastic:master Jun 14, 2021
@kindsun kindsun deleted the fix-alert-handling-of-multi-fields branch June 14, 2021 21:51
kindsun pushed a commit to kindsun/kibana that referenced this pull request Jun 14, 2021
@thomasneirynck
Copy link
Contributor

thomasneirynck commented Jun 15, 2021

@aaronjcaldwell Could you backport to 7.13 as well? There will be another patch for the 7.13 as well. Added 7.13.3 label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps][Alerting] geo-containment alerts do not support multi-fields
5 participants