Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function

[cjcenizal]

Release notes

A regression was introduced into 7.5.0 which caused a particular configuration of Threshold Watches to fail and/or erroneously trigger if they were created or edited in 7.5. If you've created or edited a Threshold Watch with a "GROUPED OVER" condition set to "top" with Kibana 7.5.0, you'll need to upgrade to a version of Kibana that contains this fix and recreate these watches. The easiest way to do this will be to go to the edit screen of the Threshold Watch in the UI and simply click the "Save" button. This will recreate the watch with the proper configuration. No other changes to the watch will be necessary on your part.

Description

Fixes #53974

This fixes a regression introduced in 7.5.0 via #43232, in which these arguments were not being provided. This resulted in two bugs, both of which would occur when when GROUPED OVER is set to top, which sets a termField and sets hasTermsAgg to true.

Bug 1: Missing termOrder causes watch to fail

Under this condition, build_input.js#L50 and build_input.js#L88 will consume termOrder in the presence of a termField. If termOrder is undefined, then order will be assigned an empty object, due to the way JSON.stringify strips out properties with undefined values.

Here's what this bug looks like in the request flyout:

Here's what the request flyout looks like now that the bug is fixed:

Bug 2: Missing hasTermsAgg results in incorrect condition script

build_transform.js and build_condition.js both depend upon hasTermsAgg to specify the correct scripts. For example, in the screenshot below the condition script should be this and the transform script should be this.

Here's what it looks like post-fix:

Due diligence

I untangled the logic on the server-side and found that the execute API consumes the watchJson getter that exists on all Watch models. The server-side ThresholdWatch model consumes serializeThresholdWatch within its version of this getter. However, the execute API is only called by the UI when the user is viewing JSON watches. I.e. the way the API is currently consumed precludes this getter from ever being called. So I couldn't really test the effects my changes have upon this logic.

Unrelated changes

In 27fcefa, I removed some unused model methods and verified this had no impact on creating, editing, simulating, or deleting watches.

Next steps

I plan to perform a tightly-scoped TS conversion to ensure that serializeThresholdWatch always receives the arguments it expects. This would have prevented this regression from originally being introduced. I'm going to do this in a separate PR because of the high level of effort required.

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[sebelga]

Code LGTM! I haven't tested locally.

[jloleysens]

I tested locally and seems to work!

Well done for implementing this fix. Regarding this comment in the PR description:

I.e. the way the API is currently consumed precludes this getter from ever being called. So I
couldn't really test the effects my changes have upon this logic.

Is there some part of watcher that is not testable? Not sure I entirely follow.

[cjcenizal]

@jloleysens Sorry about that! I'll try to clarify. There are three model classes on the server, representing different types of watches: ThresholdWatch, JsonWatch, and MonitoringWatch.

The handler for the /api/watcher/watch/execute endpoint is designed to accept any type of watch, and to consume the watchJson getter of the watch provided to it. If it were to be provided a ThresholdWatch, the watchJson getter in its implementation would end up calling serializeThresholdWatch, which is the function this PR is concerned with.

In our UI, the /api/watcher/watch/execute endpoint is consumed by the "Create/edit JSON watch" form, but not by the "Create/edit threshold watch" form (see screenshot below). This means that, though this endpoint is designed to accept threshold watches, our UI never actually sends threshold watches to it. Our UI only ever sends JSON watches to it. So there's really nothing for me to test.

I assume that if we were to add simulate functionality to the threshold watch form, we would test that the endpoint works as expected when we send threshold watch data to it.

[jloleysens]

@cjcenizal Thanks for the explanation! That makes sense!

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 5d0016e

History

• 💚 Build #19171 succeeded 5d0016e
• 💔 Build #19085 failed a5c56c9

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