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

Fix malformed geo alerts call to transformResults #98094

Merged
merged 16 commits into from
May 3, 2021

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Apr 22, 2021

This fixes a regression introduced in #93364 where the body field was incorrectly appended to currentIntervalResults. This resulted in an undefined value being passed to the function.

To test, set up a normal geo containment alert (see readme). Alerts triggered by both entering and leaving shapes were tested and found working

@kindsun kindsun requested a review from a team as a code owner April 22, 2021 21:52
@kindsun kindsun requested a review from thomasneirynck April 22, 2021 21:52
@kindsun kindsun added bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.13.0 labels Apr 22, 2021
@elasticmachine
Copy link
Contributor

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

@kindsun
Copy link
Contributor Author

kindsun commented Apr 22, 2021

@elasticmachine merge upstream

@kindsun kindsun added the release_note:skip Skip the PR/issue when compiling release notes label Apr 22, 2021
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 the forensics on this, and the quick fix.

I would look into if we can add a unit-test on the getGeoContainmentExecutor. It has some end-2-end glue-code, that is not getting covered right now. This is an opportunity to add some coverage. (it's also the API-extension point of alerting-framework, which helps).

For this test, there's already have an example of a mock on alertInstanceFactory . I think it would only require one more additional mock for the services.scopedClusterClient.asCurrentUser esClient. That mock could just return two sample ES-responses: One for the shapes (

const { body: boundaryData }: ApiResponse<Record<string, any>> = await esClient.search({
) when it is called, and one for the top-hits agg when it is called (
({ body: esResult } = await esClient.search(esQuery));
)

@kindsun kindsun force-pushed the fix-alerts-transform-results-call branch from 58f5e55 to 38a6a9b Compare April 28, 2021 21:33
@kindsun kindsun requested a review from thomasneirynck April 28, 2021 21:35
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, discussed offline. Unit test looks good, but would mock at more lower-level, the actual search of the esClient.

along the lines of:

    //
      alertServices.scopedClusterClient.callAsUser = {
        search: (...args) = {
          if (isShapeSearchCall) {
            return  sampleESShapeSearchReturn;
          } else if (isTophItsCall) {
            return sampleESTopHitsAggReturn;
          }
        }/
      };

^ this should give us more comprehensive "round trip" behavior, thx!

@kindsun kindsun requested a review from thomasneirynck April 29, 2021 22:43
@thomasneirynck
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

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.

code review, thx for adding the comprehensive tests

@kindsun kindsun merged commit 58b3c1b into elastic:master May 3, 2021
@kindsun kindsun deleted the fix-alerts-transform-results-call branch May 3, 2021 21:30
@kindsun kindsun added the v7.14.0 label May 3, 2021
kindsun pushed a commit to kindsun/kibana that referenced this pull request May 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kindsun pushed a commit to kindsun/kibana that referenced this pull request May 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kindsun pushed a commit that referenced this pull request May 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kindsun pushed a commit that referenced this pull request May 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants