Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adding HTTP Input type for monitors #82

Merged
merged 25 commits into from
Aug 1, 2019

Conversation

ann3431
Copy link
Contributor

@ann3431 ann3431 commented Jul 16, 2019

*Issue #, if available: #47

*Description of changes:

  • Adding new type of input HttpInput for monitors.

MonitorRunner class creates a HttpInputClient when collecting input results of an HttpInput.

  • Changes to README file regarding setting JAVA_HOME.

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

> > Configure project :alerting
=======================================
Elasticsearch Build Hamster says Hello!
  Gradle Version        : 5.2.1
  OS Info               : Mac OS X 10.14.5 (x86_64)
  JDK Version           : 12 (Oracle Corporation 12.0.1 [Java HotSpot(TM) 64-Bit Server VM 12.0.1+12])
  JAVA_HOME             : /Library/Java/JavaVirtualMachines/jdk-12.0.1.jdk/Contents/Home
  Random Testing Seed   : 9087A73AD72ABD1
=======================================

Deprecated Gradle features were used in this build, making it incompatible with Gradle 6.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/5.2.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1m 49s
52 actionable tasks: 30 executed, 22 up-to-date

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need some limit on response size as over-large responses can take down the cluster.

@ann3431
Copy link
Contributor Author

ann3431 commented Jul 19, 2019

We also need some limit on response size as over-large responses can take down the cluster.

@vinooamzn There is no out of box function for the client to limit response size. The way I can think of preventing this problem is to restrict the maximum timeout value so that large amount of data cannot be transmitted in limited time. Do you know any good way to limit the content size?

Revisions on several issues:
- Reusing client in `MonitorRunner`
- Allowing only port 9200 for localhost in `HttpInput`
- Fixed typo in `README.md`
- Validation only one of url or scheme+host+port+path+params is set in `HttpInput`.
- Set maximum timeout values to 60s and updated tests.
Copy link
Contributor Author

@ann3431 ann3431 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting response size to 100MB in MonitorRunner.collectInputResult().

Move port validation from HttpInput to RestIndexMonitorAction. Validation funtion ensures that the port number for localhost is restricted to the default port specified in settings.
Copy link
Contributor Author

@ann3431 ann3431 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

@lucaswin-amzn lucaswin-amzn merged commit c2004f7 into opendistro-for-elasticsearch:master Aug 1, 2019
lucaswin-amzn added a commit to lucaswin-amzn/alerting that referenced this pull request Nov 18, 2019
lucaswin-amzn added a commit that referenced this pull request Nov 19, 2019
* Revert "Adding new type of input for Monitors - HttpInput (#82)"

This reverts commit c2004f7.
dbbaughe added a commit to dbbaughe/alerting that referenced this pull request Jan 31, 2020
dbbaughe added a commit to dbbaughe/alerting that referenced this pull request Jan 31, 2020
dbbaughe added a commit that referenced this pull request Jan 31, 2020
dbbaughe added a commit that referenced this pull request Jan 31, 2020
@anirudha anirudha changed the title Adding new type of input for Monitors - HttpInput Adding HttpInput type for monitors Jul 8, 2020
@anirudha anirudha changed the title Adding HttpInput type for monitors Adding Http Input type for monitors Jul 8, 2020
@praveensameneni praveensameneni added alerting Item related to Alerting enhancement New feature or request labels Aug 5, 2020
@jcgraybill jcgraybill changed the title Adding Http Input type for monitors Adding HTTP Input type for monitors Oct 1, 2020
skkosuri-amzn pushed a commit that referenced this pull request Nov 30, 2020
* 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]>
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* 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]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
alerting Item related to Alerting enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants