Allow for http method selection in custom webhook #101

jcleezer · 2019-09-12T21:25:27Z

Issue #, if available: None

Description of changes:
This allows for the method used by the alerting custom web hook to be supplied as part of the configuration.

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

…seMethod

jcleezer · 2019-09-12T21:34:45Z

UI change is opendistro-for-elasticsearch/alerting-kibana-plugin#90

* Revert "Adding new type of input for Monitors - HttpInput (opendistro-for-elasticsearch#82)" This reverts commit c2004f7.

jcleezer · 2020-06-05T16:48:32Z

Please let me know if there's anything I can do to assist with reviewing this. This functionality would be a big improvement for my teams alerting.

sloev · 2020-06-09T08:57:50Z

we need this very much too!!!
we have an alerting api that only accepts alerts on http PUT (and the api is not owned by us) so being able to PUT alerts would be a major increase in usability for us!!!

skkosuri-amzn · 2020-11-17T01:28:49Z

@jcleezer Sorry for the delay on this.
Curious, whats the use case to support GET and DELETE?

jcleezer · 2020-11-17T01:42:36Z

Hi @skkosuri-amzn, admittedly I added this to get support for PUT. It was minimal effort to add GET and DELETE as well and I figured it could possibly aid other use cases.

skkosuri-amzn · 2020-11-17T01:44:30Z

@jcleezer Can we limit only for POST, PUT and PATCH?

jcleezer · 2020-11-17T18:42:03Z

@skkosuri-amzn works for me. I've updated the PR with GET and DELETE removed.

...com/amazon/opendistroforelasticsearch/alerting/destination/message/CustomWebhookMessage.java

...ava/com/amazon/opendistroforelasticsearch/alerting/destination/CustomWebhookMessageTest.java

codecov · 2020-11-17T19:28:35Z

Codecov Report

Merging #101 (bf8add9) into master (f7a3982) will increase coverage by 0.12%.
The diff coverage is 87.61%.

@@             Coverage Diff              @@
##             master     #101      +/-   ##
============================================
+ Coverage     80.17%   80.29%   +0.12%     
- Complexity      196      202       +6     
============================================
  Files           151      151              
  Lines          5165     5192      +27     
  Branches        673      679       +6     
============================================
+ Hits           4141     4169      +28     
+ Misses          666      661       -5     
- Partials        358      362       +4

Impacted Files	Coverage Δ	Complexity Δ
...forelasticsearch/alerting/model/destination/SNS.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...arch/alerting/core/schedule/JobSchedulerMetrics.kt	`81.25% <0.00%> (ø)`	`6.00 <0.00> (ø)`
...ting/destination/message/CustomWebhookMessage.java	`67.18% <55.55%> (-1.91%)`	`14.00 <1.00> (+3.00)`	⬇️
...roforelasticsearch/alerting/alerts/AlertIndices.kt	`65.64% <60.00%> (-0.50%)`	`0.00 <0.00> (ø)`
...ting/destination/client/DestinationHttpClient.java	`81.25% <73.33%> (-2.32%)`	`17.00 <3.00> (+3.00)`	⬇️
...icsearch/alerting/model/destination/Destination.kt	`76.36% <83.33%> (+1.36%)`	`0.00 <0.00> (ø)`
...endistroforelasticsearch/alerting/MonitorRunner.kt	`78.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...stroforelasticsearch/alerting/alerts/AlertError.kt	`80.95% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...stroforelasticsearch/alerting/alerts/AlertMover.kt	`72.72% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...sticsearch/alerting/model/ActionExecutionResult.kt	`87.09% <100.00%> (ø)`	`0.00 <0.00> (ø)`
... and 32 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 f7a3982...5617557. Read the comment docs.

skkosuri-amzn · 2020-11-17T19:53:41Z

Thanks @jcleezer

skkosuri-amzn · 2020-11-18T18:03:04Z

Tagged this for next release.

lezzago · 2020-11-25T17:44:57Z

...ava/com/amazon/opendistroforelasticsearch/alerting/destination/CustomWebhookMessageTest.java

+            DestinationHttpClient httpClient = new DestinationHttpClient();
+            httpClient.setHttpClient(mockHttpClient);
+            CustomWebhookDestinationFactory customDestinationFactory = new CustomWebhookDestinationFactory();
+            customDestinationFactory.setClient(httpClient);

-        DestinationFactoryProvider.setFactory(DestinationType.CUSTOMWEBHOOK, customDestinationFactory);
+            DestinationFactoryProvider.setFactory(DestinationType.CUSTOMWEBHOOK, customDestinationFactory);

-        Map<String, String> queryParams = new HashMap<String, String>();
-        queryParams.put("token", "R2x1UlN4ZHF8MXxxVFJpelJNVDgzdGNwMnVRenJwRFBHUkR0NlhROWhXOVVTZXpiTWx2azVr");
+            Map<String, String> queryParams = new HashMap<String, String>();
+            queryParams.put("token", "R2x1UlN4ZHF8MXxxVFJpelJNVDgzdGNwMnVRenJwRFBHUkR0NlhROWhXOVVTZXpiTWx2azVr");

        String message = "{\"Content\":\"Message gughjhjlkh Body emoji test: :) :+1: " +
                "link test: http://sample.com email test: [email protected] " +
                "All member callout: @All All Present member callout: @Present\"}";
        BaseMessage bm = new CustomWebhookMessage.Builder("abc").withHost("hooks.chime.aws").
                withPath("incomingwebhooks/383c0e2b-d028-44f4-8d38-696754bc4574").
-                withMessage(message).
+                withMessage(message).withMethod(method).
                withQueryParams(queryParams).build();
        DestinationResponse actualCustomResponse = (DestinationResponse) Notification.publish(bm);

-        assertEquals(expectedCustomWebhookResponse.getResponseContent(), actualCustomResponse.getResponseContent());
-        assertEquals(expectedCustomWebhookResponse.getStatusCode(), actualCustomResponse.getStatusCode());
-    }
+            assertEquals(expectedCustomWebhookResponse.getResponseContent(), actualCustomResponse.getResponseContent());
+            assertEquals(expectedCustomWebhookResponse.getStatusCode(), actualCustomResponse.getStatusCode());
+        }


Minor Nitpick: Please remove the extra spacing, so the different code lines are on a consistent indent.

Thanks! I believe I caught all the places where that somehow got changed.

lezzago

Minor Nitpick on line indenting. I went through everything in the code and these are the remaining line indents that are incorrect.

lezzago · 2020-11-30T16:06:08Z

...ava/com/amazon/opendistroforelasticsearch/alerting/destination/CustomWebhookMessageTest.java


 import java.util.HashMap;
 import java.util.Map;

 import static org.junit.Assert.assertEquals;

+
+    @RunWith(Parameterized.class)


This should be the same indent as line 46

Thanks! Good eye, I believe these have been fixed.

lezzago · 2020-11-30T16:06:42Z

...ava/com/amazon/opendistroforelasticsearch/alerting/destination/CustomWebhookMessageTest.java

+        @Parameterized.Parameters(name = "Param: {0}={1}")
+        public static Object[][] params() {
+            return new Object[][]{
+                    {"POST", HttpPost.class},
+                    {"PUT", HttpPut.class},
+                    {"PATCH", HttpPatch.class},
+            };
+        }
+
+        @Parameterized.Parameter(0)
+        public String method;
+
+        @Parameterized.Parameter(1)
+        public Class<HttpUriRequest> expectedHttpClass;


This should be the same indent as line 62

lezzago · 2020-11-30T16:45:32Z

Thanks @jcleezer

* Add http method to custom webhook * Remove unused line * Opendistro 1.3 * Revert "Adding new type of input for Monitors - HttpInput (#82)" This reverts commit c2004f7. * fix alerting stats API (#129) * Bump plugin version (#131) * Restrict custom http alert method to POST PUT and PATCH * Fix indenting * Fix indention * Fix additional indention Co-authored-by: Jason Leezer <[email protected]> Co-authored-by: Lucas Winkelmann <[email protected]> Co-authored-by: Jinsoo <[email protected]> Co-authored-by: Mohammad Qureshi <[email protected]>

Jason Leezer added 3 commits September 12, 2019 16:19

Add http method to custom webhook

c7f5e53

Merge remote-tracking branch 'upstream/master' into customWebhookChoo…

e5bfe68

…seMethod

Remove unused line

46f277a

jcleezer mentioned this pull request Sep 12, 2019

Allow for http method selection in custom webhook opendistro-for-elasticsearch/alerting-kibana-plugin#90

Merged

stevensideyliu assigned stevensideyliu and unassigned stevensideyliu Oct 7, 2019

lucaswin-amzn and others added 6 commits November 18, 2019 16:09

Opendistro 1.3

edd6766

* Revert "Adding new type of input for Monitors - HttpInput (opendistro-for-elasticsearch#82)" This reverts commit c2004f7.

fix alerting stats API (opendistro-for-elasticsearch#129)

f9670c0

Bump plugin version (opendistro-for-elasticsearch#131)

1fe3ea8

Merge tag 'v1.3.0.0' into customWebhookChooseMethod

47f94fb

Merge tag 'v1.3.0.1' into customWebhookChooseMethod

17f982b

Merge branch 'master' into customWebhookChooseMethod

3eda757

Restrict custom http alert method to POST PUT and PATCH

ac96250

Merge master into branch.

a509ea2

skkosuri-amzn reviewed Nov 17, 2020

View reviewed changes

...com/amazon/opendistroforelasticsearch/alerting/destination/message/CustomWebhookMessage.java Outdated Show resolved Hide resolved

skkosuri-amzn reviewed Nov 17, 2020

View reviewed changes

...ava/com/amazon/opendistroforelasticsearch/alerting/destination/CustomWebhookMessageTest.java Outdated Show resolved Hide resolved

Fix conflicts and address feedback

be6c067

skkosuri-amzn previously approved these changes Nov 17, 2020

View reviewed changes

skkosuri-amzn added the next release Targeted for next ODFE release label Nov 18, 2020

skkosuri-amzn requested a review from lezzago November 24, 2020 02:19

bowenlan-amzn added the enhancement New feature or request label Nov 25, 2020

lezzago reviewed Nov 25, 2020

View reviewed changes

Fix indenting

76bee5f

jcleezer dismissed skkosuri-amzn’s stale review via 76bee5f November 30, 2020 15:05

Fix indention

f0ab022

lezzago reviewed Nov 30, 2020

View reviewed changes

Fix additional indention

5617557

lezzago approved these changes Nov 30, 2020

View reviewed changes

skkosuri-amzn approved these changes Nov 30, 2020

View reviewed changes

skkosuri-amzn merged commit 6fc0cde into opendistro-for-elasticsearch:master Nov 30, 2020

skkosuri-amzn mentioned this pull request Nov 30, 2020

Allow for http method selection in custom webhook #309

Closed

qreshi mentioned this pull request Nov 16, 2021

Add HTTP method option on custom webhook opensearch-project/alerting#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for http method selection in custom webhook #101

Allow for http method selection in custom webhook #101

jcleezer commented Sep 12, 2019

jcleezer commented Sep 12, 2019

jcleezer commented Jun 5, 2020

sloev commented Jun 9, 2020

skkosuri-amzn commented Nov 17, 2020 •

edited

Loading

jcleezer commented Nov 17, 2020

skkosuri-amzn commented Nov 17, 2020

jcleezer commented Nov 17, 2020

codecov bot commented Nov 17, 2020 •

edited

Loading

skkosuri-amzn commented Nov 17, 2020

skkosuri-amzn commented Nov 18, 2020

lezzago Nov 25, 2020 •

edited

Loading

jcleezer Nov 30, 2020

lezzago left a comment

lezzago Nov 30, 2020

jcleezer Nov 30, 2020

lezzago Nov 30, 2020

lezzago commented Nov 30, 2020

Allow for http method selection in custom webhook #101

Allow for http method selection in custom webhook #101

Conversation

jcleezer commented Sep 12, 2019

jcleezer commented Sep 12, 2019

jcleezer commented Jun 5, 2020

sloev commented Jun 9, 2020

skkosuri-amzn commented Nov 17, 2020 • edited Loading

jcleezer commented Nov 17, 2020

skkosuri-amzn commented Nov 17, 2020

jcleezer commented Nov 17, 2020

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

skkosuri-amzn commented Nov 17, 2020

skkosuri-amzn commented Nov 18, 2020

lezzago Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

jcleezer Nov 30, 2020

Choose a reason for hiding this comment

lezzago left a comment

Choose a reason for hiding this comment

lezzago Nov 30, 2020

Choose a reason for hiding this comment

jcleezer Nov 30, 2020

Choose a reason for hiding this comment

lezzago Nov 30, 2020

Choose a reason for hiding this comment

lezzago commented Nov 30, 2020

skkosuri-amzn commented Nov 17, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading

lezzago Nov 25, 2020 •

edited

Loading