Add getDestinations and getAlerts API #260

lezzago · 2020-09-28T19:50:00Z

Issue #, if available:
#215
Description of changes:
#215

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

…er indexes

codecov · 2020-09-28T23:55:57Z

Codecov Report

Merging #260 into master will increase coverage by 1.04%.
The diff coverage is 93.15%.

@@             Coverage Diff              @@
##             master     #260      +/-   ##
============================================
+ Coverage     79.34%   80.39%   +1.04%     
- Complexity      197      199       +2     
============================================
  Files           138      149      +11     
  Lines          4522     4810     +288     
  Branches        614      635      +21     
============================================
+ Hits           3588     3867     +279     
- Misses          596      597       +1     
- Partials        338      346       +8

Impacted Files	Coverage Δ	Complexity Δ
...orelasticsearch/alerting/action/GetAlertsAction.kt	`66.66% <66.66%> (ø)`	`0.00 <0.00> (?)`
...ticsearch/alerting/action/GetDestinationsAction.kt	`66.66% <66.66%> (ø)`	`0.00 <0.00> (?)`
...erting/transport/TransportGetDestinationsAction.kt	`86.53% <86.53%> (ø)`	`0.00 <0.00> (?)`
...rch/alerting/transport/TransportGetAlertsAction.kt	`86.66% <86.66%> (ø)`	`0.00 <0.00> (?)`
...icsearch/alerting/model/destination/Destination.kt	`73.88% <87.50%> (+3.95%)`	`0.00 <0.00> (ø)`
...elasticsearch/alerting/action/GetAlertsResponse.kt	`93.33% <93.33%> (ø)`	`0.00 <0.00> (?)`
.../alerting/resthandler/RestGetDestinationsAction.kt	`93.93% <93.93%> (ø)`	`0.00 <0.00> (?)`
...ndistroforelasticsearch/alerting/AlertingPlugin.kt	`91.48% <100.00%> (+0.37%)`	`0.00 <0.00> (ø)`
...relasticsearch/alerting/action/GetAlertsRequest.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...icsearch/alerting/action/GetDestinationsRequest.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
... and 20 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 39a13d3...841a942. Read the comment docs.

skkosuri-amzn

Added my comments.

skkosuri-amzn · 2020-09-29T02:30:40Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunner.kt

@@ -185,6 +185,21 @@ class MonitorRunner(
    }

    suspend fun runMonitor(monitor: Monitor, periodStart: Instant, periodEnd: Instant, dryrun: Boolean = false): MonitorRunResult {
+        /*


Looks like you picked inject roles changes. Do we need this change for getDest, getAlerts?

Removed. I accidentally picked it up

skkosuri-amzn · 2020-09-29T02:34:54Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/Alert.kt

@@ -121,9 +121,9 @@ data class Alert(

    companion object {

-        const val ALERT_ID_FIELD = "id"
+        const val ALERT_ID_FIELD = "alert_id"


Whats the reason behind changing the name from id to alert_id ?

We need to insert the same string into alert_mapping.json. In the file, there is monitor_id and trigger_id, so I wanted to follow the same format and ensure it is clear what the id is for.

skkosuri-amzn · 2020-09-29T02:36:20Z

.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/destination/Destination.kt

                type = sin.readEnum(DestinationType::class.java),
                name = sin.readString(),
                user = if (sin.readBoolean()) {
-                    User(sin)
+                    User.readFrom(sin)


Looks like you over wrote my change ;) . User(sin)

When I cherry picked changes, it looked like it was changed to User.readFrom(sin). Just changed it back

skkosuri-amzn · 2020-09-29T02:38:34Z

...tlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestGetDestinationsAction.kt

+        return listOf(
+                // Get a specific destination
+                RestHandler.Route(RestRequest.Method.GET, "${AlertingPlugin.DESTINATION_BASE_URI}/{destinationID}"),
+                RestHandler.Route(RestRequest.Method.GET, "${AlertingPlugin.DESTINATION_BASE_URI}/all")


is all a standard REST way? Cant we leave GET /destinations/

Thanks for pointing that out. Meant to change it to something better later on after testing.

skkosuri-amzn · 2020-09-29T02:40:25Z

...kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestSearchMonitorAction.kt

+        log.debug("${request.method()} ${AlertingPlugin.MONITOR_BASE_URI}/_search")
+
+        val index = request.param("index", SCHEDULED_JOBS_INDEX)
+


How this change is related to GET /destinations GET /alerts?

I had included it because it was needed for the security changes. I will remove this change since it is already included in ur PR.

ftianli-amzn · 2020-09-29T17:46:54Z

...ing/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/DestinationTests.kt

@@ -135,7 +135,7 @@ class DestinationTests : ESTestCase() {
    }

    fun `test slack destination create using stream`() {
-        val chimeDest = Destination("2345", 1L, 2, DestinationType.SLACK, "TestSlackDest",
+        val chimeDest = Destination("2345", 1L, 2, 1, 1, DestinationType.SLACK, "TestSlackDest",


The variable is a little inaccurate, including the one for "custom webhook" in below test, but looks like it's a change in a past PR.

thanks for pointing that out. I fixed them

Thank you 👍

* Add getDestination(s) rest api * remove security context for GetDestinations api * replaces try-finally with use for all auto-closables * add getAlerts API * minor cleanup to get-alerts * fix sorting and pagination. Also allow searchMonitor to search in other indexes * fix sorting, searching, and filtering * add opendistro to GetDestinations action name * Cleanup code and add unit and integ tests * cleanup some of the read streaminput code * cleanup code more * cleanup tests * fix unit test * remove extra stuff * fix mappings * fix test names Co-authored-by: skkosuri-amzn <[email protected]>

lezzago and others added 11 commits September 28, 2020 11:55

Add getDestination(s) rest api

1c7a425

remove security context for GetDestinations api

8b0385d

replaces try-finally with use for all auto-closables

5955a87

add getAlerts API

b1efff0

minor cleanup to get-alerts

2df602d

fix sorting and pagination. Also allow searchMonitor to search in oth…

4ab9a98

…er indexes

fix sorting, searching, and filtering

6bcaab6

add opendistro to GetDestinations action name

f7605a8

Cleanup code and add unit and integ tests

4fe5e37

cleanup some of the read streaminput code

e13fc77

cleanup code more

19375bf

lezzago changed the title ~~Ashisagr dev~~ Add getDestinations and getAlerts API Sep 28, 2020

cleanup tests

92463cf

lezzago closed this Sep 28, 2020

lezzago reopened this Sep 28, 2020

lezzago requested review from skkosuri-amzn and qreshi September 28, 2020 21:13

lezzago marked this pull request as ready for review September 28, 2020 21:13

lezzago self-assigned this Sep 28, 2020

lezzago requested a review from ftianli-amzn September 28, 2020 21:13

fix unit test

952ed96

skkosuri-amzn reviewed Sep 29, 2020

View reviewed changes

remove extra stuff

3f4e592

ftianli-amzn reviewed Sep 29, 2020

View reviewed changes

lezzago added 2 commits September 29, 2020 11:27

fix mappings

447e272

fix test names

e5c25ed

skkosuri-amzn previously approved these changes Sep 29, 2020

View reviewed changes

ftianli-amzn previously approved these changes Sep 29, 2020

View reviewed changes

Merge branch 'master' into ashisagr-dev

841a942

lezzago dismissed stale reviews from ftianli-amzn and skkosuri-amzn via 841a942 September 30, 2020 00:19

ftianli-amzn approved these changes Sep 30, 2020

View reviewed changes

skkosuri-amzn approved these changes Sep 30, 2020

View reviewed changes

lezzago merged commit 01269ec into master Sep 30, 2020

lezzago deleted the ashisagr-dev branch December 4, 2020 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getDestinations and getAlerts API #260

Add getDestinations and getAlerts API #260

lezzago commented Sep 28, 2020

codecov bot commented Sep 28, 2020 •

edited

Loading

skkosuri-amzn left a comment

skkosuri-amzn Sep 29, 2020

lezzago Sep 29, 2020

skkosuri-amzn Sep 29, 2020

lezzago Sep 29, 2020

skkosuri-amzn Sep 29, 2020

lezzago Sep 29, 2020

skkosuri-amzn Sep 29, 2020

lezzago Sep 29, 2020

skkosuri-amzn Sep 29, 2020

lezzago Sep 29, 2020

ftianli-amzn Sep 29, 2020

lezzago Sep 29, 2020

ftianli-amzn Sep 29, 2020

		log.debug("${request.method()} ${AlertingPlugin.MONITOR_BASE_URI}/_search")

		val index = request.param("index", SCHEDULED_JOBS_INDEX)

Add getDestinations and getAlerts API #260

Add getDestinations and getAlerts API #260

Conversation

lezzago commented Sep 28, 2020

codecov bot commented Sep 28, 2020 • edited Loading

Codecov Report

skkosuri-amzn left a 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

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 Sep 28, 2020 •

edited

Loading