Add actions to create, execute, get monitors api #240

skkosuri-amzn · 2020-08-20T22:29:49Z

Issue #, if available:
#232
Description of changes:
This to add actions to create/modify , execute, get monitor.

#232
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2020-08-20T22:33:58Z

Codecov Report

Merging #240 into master will increase coverage by 1.28%.
The diff coverage is 82.84%.

@@             Coverage Diff              @@
##             master     #240      +/-   ##
============================================
+ Coverage     77.51%   78.79%   +1.28%     
  Complexity      164      164              
============================================
  Files            75       91      +16     
  Lines          2948     3249     +301     
  Branches        442      458      +16     
============================================
+ Hits           2285     2560     +275     
- Misses          433      446      +13     
- Partials        230      243      +13

Impacted Files	Coverage Δ	Complexity Δ
...alerting/transport/TransportSearchMonitorAction.kt	`57.14% <ø> (ø)`	`0.00 <0.00> (ø)`
...lerting/transport/TransportExecuteMonitorAction.kt	`64.51% <64.51%> (ø)`	`0.00 <0.00> (?)`
.../alerting/transport/TransportIndexMonitorAction.kt	`66.08% <66.08%> (ø)`	`0.00 <0.00> (?)`
...sticsearch/alerting/action/ExecuteMonitorAction.kt	`66.66% <66.66%> (ø)`	`0.00 <0.00> (?)`
...relasticsearch/alerting/action/GetMonitorAction.kt	`66.66% <66.66%> (ø)`	`0.00 <0.00> (?)`
...lasticsearch/alerting/action/IndexMonitorAction.kt	`66.66% <66.66%> (ø)`	`0.00 <0.00> (?)`
...orelasticsearch/alerting/model/MonitorRunResult.kt	`85.12% <75.00%> (+1.36%)`	`0.00 <0.00> (ø)`
...ch/alerting/transport/TransportGetMonitorAction.kt	`85.00% <85.00%> (ø)`	`0.00 <0.00> (?)`
...lasticsearch/alerting/action/GetMonitorResponse.kt	`92.50% <92.50%> (ø)`	`0.00 <0.00> (?)`
...sticsearch/alerting/action/IndexMonitorResponse.kt	`94.11% <94.11%> (ø)`	`0.00 <0.00> (?)`
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4119ef1...fb873f6. Read the comment docs.

lezzago · 2020-08-24T21:50:03Z

.../src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/GetMonitorResponse.kt

+        sin.readEnum(RestStatus::class.java), // RestStatus
+        if (sin.readBoolean()) {
+            Monitor.readFrom(sin) // monitor
+        } else null


What is the purpose for this?

This response is used for GET or HEAD request. With HEAD request we wont have monitor object in response. This checks if monitor is present. Also refer to writeTo
if (monitor != null) { out.writeBoolean(true) monitor?.writeTo(out) } else { out.writeBoolean(false) }

Misread the code, just realized that this is inside a constructor call. Makes sense why this is done

lezzago · 2020-08-26T16:28:23Z

...in/com/amazon/opendistroforelasticsearch/alerting/transport/TransportExecuteMonitorAction.kt

+                            ElasticsearchStatusException("Can't find monitor with id: ${response.id}", RestStatus.NOT_FOUND)
+                        )
+                    }
+                    if (!response.isSourceEmpty) {


This statement was not there before in the RestExecuteMonitorAction.kt file. Is this fixing a bug that was there before?

Good catch. I added this extra check here to be extra safe. We were doing that in RestGetMonitorAction

alerting/alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestGetMonitorAction.kt

Line 91 in 9f405c2

if (!response.isSourceEmpty) {

lezzago · 2020-08-26T16:37:47Z

...tlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt

+    init {
+        clusterService.clusterSettings.addSettingsUpdateConsumer(ALERTING_MAX_MONITORS) { maxMonitors = it }
+        clusterService.clusterSettings.addSettingsUpdateConsumer(REQUEST_TIMEOUT) { requestTimeout = it }
+        clusterService.clusterSettings.addSettingsUpdateConsumer(INDEX_TIMEOUT) { indexTimeout = it }
+        clusterService.clusterSettings.addSettingsUpdateConsumer(MAX_ACTION_THROTTLE_VALUE) { maxActionThrottle = it }
+    }


Why does this need to be set if it is done in RestIndexMonitorAction.kt?

Good catch. I missed to remove this from RestIndexMonitorAction. Will fix it.

lezzago · 2020-08-26T17:25:56Z

...t/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/ExecuteMonitorRequestTests.kt

+        req.writeTo(out)
+        val sin = StreamInput.wrap(out.bytes().toBytesRef().bytes)
+        val newReq = ExecuteMonitorRequest(sin)
+        assertNull(newReq.monitorId)


Please compare this with the original monitorId

In this test the monitorId is null, so left this to check assertNull. Added a new check below to compare monitor.

lezzago · 2020-08-26T17:28:51Z

...test/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/GetMonitorResponseTests.kt

+                Monitor(
+                "123",
+                        0L, "test-monitor", true, cronSchedule, Instant.now(),
+                        Instant.now(), 0, mutableListOf(), mutableListOf(), mutableMapOf()
+
+                ))


Please fix formatting so it is consistent for the input attributes

Fixed formatting.

lezzago · 2020-08-26T17:30:08Z

...st/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/IndexMonitorResponseTests.kt

+                Monitor(
+                "123",
+                        0L, "test-monitor", true, cronSchedule, Instant.now(),
+                        Instant.now(), 0, mutableListOf(), mutableListOf(), mutableMapOf()
+
+                ))


Please fix formatting so it is consistent for the input attributes

Fixed formatting.

Add actions to create, execute, get monitors api

551930a

skkosuri-amzn requested review from ftianli-amzn, lezzago and qreshi August 20, 2020 22:37

skkosuri-amzn added 2 commits August 21, 2020 10:12

Fixed unittests

a3ac0e7

cleanup warnings

8009312

lezzago reviewed Aug 26, 2020

View reviewed changes

Addressed review comments

fb873f6

lezzago approved these changes Aug 26, 2020

View reviewed changes

ftianli-amzn approved these changes Aug 26, 2020

View reviewed changes

skkosuri-amzn merged commit 7320bc6 into master Aug 26, 2020

bowenlan-amzn added the enhancement New feature or request label Sep 8, 2020

skkosuri-amzn mentioned this pull request Sep 17, 2020

Integrate Security with Alerting #6

Closed

qreshi deleted the monitor-apis branch December 19, 2020 00:47

tlfeng pushed a commit that referenced this pull request Feb 6, 2021

Add actions to create, execute, get monitors api (#240)

2a05961

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add actions to create, execute, get monitors api #240

Add actions to create, execute, get monitors api #240

skkosuri-amzn commented Aug 20, 2020

codecov bot commented Aug 20, 2020 •

edited

Loading

lezzago Aug 24, 2020

skkosuri-amzn Aug 26, 2020

lezzago Aug 26, 2020

lezzago Aug 26, 2020

skkosuri-amzn Aug 26, 2020 •

edited

Loading

lezzago Aug 26, 2020

skkosuri-amzn Aug 26, 2020

lezzago Aug 26, 2020

skkosuri-amzn Aug 26, 2020

lezzago Aug 26, 2020

skkosuri-amzn Aug 26, 2020

lezzago Aug 26, 2020

skkosuri-amzn Aug 26, 2020

Add actions to create, execute, get monitors api #240

Add actions to create, execute, get monitors api #240

Conversation

skkosuri-amzn commented Aug 20, 2020

codecov bot commented Aug 20, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skkosuri-amzn Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2020 •

edited

Loading

skkosuri-amzn Aug 26, 2020 •

edited

Loading